Skip to content

Add an IDE storage service benchmark#39105

Merged
sharwell merged 6 commits intodotnet:masterfrom
CyrusNajmabadi:sqliteBenchmark
Oct 15, 2019
Merged

Add an IDE storage service benchmark#39105
sharwell merged 6 commits intodotnet:masterfrom
CyrusNajmabadi:sqliteBenchmark

Conversation

@CyrusNajmabadi
Copy link
Contributor

This helps enable us to make changes to the storage service and ensure that we're not regressing perf.

@CyrusNajmabadi CyrusNajmabadi requested review from a team as code owners October 7, 2019 19:00
@CyrusNajmabadi
Copy link
Contributor Author

Tagging @dotnet/roslyn-ide @sharwell . Extracted this out of: #36970

@jinujoseph jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Oct 7, 2019
@CyrusNajmabadi
Copy link
Contributor Author

@sharwell can you merge in?

@CyrusNajmabadi
Copy link
Contributor Author

@sharwell @jinujoseph can we move forward on this? This is just a perf benchmark to help when changing the sql persistence layer. Thanks!

@CyrusNajmabadi
Copy link
Contributor Author

@sharwell Can you please merge in? Thanks!

Copy link
Contributor

@sharwell sharwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good though I had a couple questions.

return GetStorageWorker(solution);
}

internal IChecksummedPersistentStorage GetStorageWorker(Solution solution)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ Is there a reason this method is needed? I thought setting the solution size threshold and database location would be enough to avoid the use of NoOpPersistentStorage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately no. There is also an additional check that the workspace has a valid solution-file. For an adhoc workspace this check fails and we don't create the db. I don't want to change anything in real code about htat just now.


private static readonly byte[] s_bytes = new byte[1000];

[Benchmark(Baseline = true)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 I don't believe Baseline = true is required here.

[Benchmark(Baseline = true)]
public Task Perf()
{
var tasks = new List<Task>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can avoid a few unnecessary array allocations as the list grows.

Suggested change
var tasks = new List<Task>();
var tasks = new List<Task>(1000);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enh :)

Copy link
Contributor

@sharwell sharwell Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably get fixed since it's inside the benchmark measurement loop so it impacts the results. We don't have to avoid all allocations but this one is pretty easy.

_useExportProviderAttribute.After(null);
}

private static readonly byte[] s_bytes = new byte[1000];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ Do we have a way to get a distribution of the size of writes we expect to see for e.g. Roslyn?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it depends on the index and if we're in teh building phase or not. indices are rewritten depending on how sensitive they are to checksum changes. So, for example, our syntactic indices are extremely read-heavy and light on writes (unless, of course, you do something to touch a ton of files on disk).

however, semantic indices are very write heavy as many changes can cause a cascading "recompute teh index" pass to happen when something changes.

Last time i looked into this there were patterns all over the place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm specifically interested in the size of the writes. We use an array pool for write buffers if the writes are less than 4K (with this benchmark fitting inside that pool), but we don't pool arrays for larger writes. I'm curious what size writes we have in practice.

@sharwell sharwell merged commit 1de29ff into dotnet:master Oct 15, 2019
@CyrusNajmabadi CyrusNajmabadi deleted the sqliteBenchmark branch January 25, 2020 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants