[WIP] Attempt to reduce allocations coming in from SQLite backend by Therzok · Pull Request #34790 · dotnet/roslyn

Therzok · 2019-04-05T13:27:54Z

Therzok · 2019-04-05T13:28:30Z

There's a few more paths to optimize. Is this approach correct until now? I'm trying to figure out how to reduce async continuation allocations.

src/Workspaces/Core/Portable/Storage/SQLite/Interop/SqlConnection.cs

src/Workspaces/Core/Portable/Storage/SQLite/SQLitePersistentStorage_BulkPopulateIds.cs

Therzok · 2019-04-05T15:12:51Z

cc @heejaechang

CyrusNajmabadi · 2019-04-05T19:48:06Z

Can you please include me on all sqlite changes. Thanks!

src/Workspaces/Core/Portable/Storage/SQLite/Interop/SqlConnection.cs

Therzok · 2019-04-06T15:49:33Z

Styling fixed. I'll take a look at the other allocation sources.

CyrusNajmabadi · 2019-04-06T17:59:49Z

src/Compilers/Core/Portable/InternalUtilities/SemaphoreSlimExtensions.cs

should this be ValueTask<SemaphoreDisposer>. Same question with a lot of your other changes?

Could be. But given await SemaphoreSlim.WaitAsync() is not going to finish synchronously, this won't finish synchronously, so we still end up creating a Task.

sure, in the async case, we'd allocate. But my gut makes me think in most cases we wouldn't be contentious. but maybe that should be examined in another PR :)

Yeah, we'll get some benchmarks for this to see how it performs.

src/Compilers/Core/Portable/InternalUtilities/SemaphoreSlimExtensions.cs

src/Workspaces/Core/Portable/Storage/SQLite/Interop/SqlConnection.cs

Therzok · 2019-04-06T21:56:30Z

Still working on more allocation paths, will add more commits until we get benchmarks for this.

Therzok · 2019-04-08T14:27:37Z

Not sure how to fix FlushSpecificWrites, as it's allocating a lot of capture classes.

src/Compilers/Core/Portable/InternalUtilities/SemaphoreSlimExtensions.cs

CyrusNajmabadi · 2019-04-08T17:35:12Z

Not sure how to fix FlushSpecificWrites, as it's allocating a lot of capture classes.

maybe we should step back. do we know why this is happening? i think we often try to fix the symptom, but not necessarily the root cause.

CyrusNajmabadi · 2019-04-08T17:37:43Z

Not sure how to fix FlushSpecificWrites, as it's allocating a lot of capture classes.

in terms of just fixing this, we could move to a model where we map from Key to a value-tuple of cached action, and the data the action would perform on. It will take a little code-finagling. but should be possible.

…osableWaitAsync

…sk completes synchronously

…path. The ContinueWith was still allocating another task instance.

sharwell

This change was superseded by a number of other recent changes. A huge thank you to @Therzok for the initial investigation, then multiple iterations of work to produce a viable and reproducible benchmark, and confirmation of the results from each individual change.

sharwell · 2019-06-08T22:09:14Z

src/Compilers/Core/Portable/InternalUtilities/SemaphoreSlimExtensions.cs

        }

-        public async static Task<SemaphoreDisposer> DisposableWaitAsync(this SemaphoreSlim semaphore, CancellationToken cancellationToken = default)
+        public static async ValueTask<SemaphoreDisposer> DisposableWaitAsync(this SemaphoreSlim semaphore, CancellationToken cancellationToken = default)


📝 This change was merged in #36128, largely due to the groundwork laid by this pull request

sharwell · 2019-06-08T22:10:28Z

src/Workspaces/Core/Portable/Storage/SQLite/Interop/SqlConnection.cs

        }

-        public void RunInTransaction(Action action)
+        public TResult RunInTransaction<T, TResult>(T userData, Func<T, TResult> action)


📝 The equivalent of this change was merged in #36127, largely due to the groundwork laid by this pull request

Therzok requested a review from a team as a code owner April 5, 2019 13:27

Therzok commented Apr 5, 2019

View reviewed changes