Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document state storage refactoring #51420

Closed
wants to merge 2 commits into from
Closed

Document state storage refactoring #51420

wants to merge 2 commits into from

Conversation

tmat
Copy link
Member

@tmat tmat commented Feb 23, 2021

Alternative to #50298. Keeping the previous data structure: ImmutableList and ImmutableSortedDictionary.

Before

Method DocumentCount Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
Project.DocumentIds 0 24.42 ns 0.194 ns 0.181 ns - - - -
Project.Documents 0 78.83 ns 0.503 ns 0.446 ns 0.0772 - - 128 B
Project.DocumentIds 100 8,172.07 ns 8.030 ns 7.118 ns 0.0305 - - 72 B
Project.Documents 100 24,651.12 ns 98.105 ns 91.767 ns 0.0916 - - 201 B
Solution.WithDocumentText 100 12,661.10 ns 252.509 ns 319.343 ns 0.8240 0.2594 - 5172 B
Project.DocumentIds 10000 859,548.56 ns 399.791 ns 373.965 ns - - - 80 B
Project.Documents 10000 4,203,826.43 ns 19,733.173 ns 18,458.422 ns - - - 256 B
Solution.WithDocumentText 10000 13,574.50 ns 270.030 ns 500.518 ns 0.8545 0.2289 - 5425 B

After

Method DocumentCount Mean Error StdDev Median Gen 0 Gen 1 Gen 2 Allocated
Project.DocumentIds 0 23.34 ns 0.494 ns 0.891 ns 22.91 ns - - - -
Project.Documents 0 77.22 ns 0.537 ns 0.502 ns 77.31 ns 0.0772 - - 128 B
Project.DocumentIds 100 8,159.41 ns 10.113 ns 8.965 ns 8,157.79 ns 0.0305 - - 72 B
Project.Documents 100 16,356.60 ns 97.365 ns 91.076 ns 16,382.64 ns 0.0916 - - 201 B
Solution.WithDocumentText 100 14,436.90 ns 355.846 ns 1,015.248 ns 14,392.68 ns 0.8240 0.2136 - 5285 B
Project.DocumentIds 10000 863,988.25 ns 1,297.511 ns 1,213.693 ns 863,625.88 ns - - - 80 B
Project.Documents 10000 1,861,933.16 ns 11,538.506 ns 10,793.125 ns 1,859,064.65 ns - - - 208 B
Solution.WithDocumentText 10000 14,088.55 ns 281.041 ns 453.828 ns 14,003.49 ns 0.8698 0.2289 - 5539 B

Measured on

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042
AMD Ryzen Threadripper 3960X, 1 CPU, 48 logical and 24 physical cores
  [Host]     : .NET Framework 4.8 (4.8.4300.0), X64 RyuJIT
  DefaultJob : .NET Framework 4.8 (4.8.4300.0), X64 RyuJIT

@tmat tmat requested a review from a team as a code owner February 23, 2021 20:25
Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

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

mainly looks good, a few minor things

the WithDocumentText benchmarks looks like they increased more than the error, is that expected / problematic

@tmat
Copy link
Member Author

tmat commented Feb 25, 2021

the WithDocumentText benchmarks looks like they increased more than the error, is that expected / problematic

The difference is not significant. Modifying operations are much less important than read operations.

@tmat
Copy link
Member Author

tmat commented Mar 1, 2021

Oops, I have accidentally merged the commits of this PR in a follow-up PR: #51526

@dibarbet I'll send another PR that implements your suggestions.

Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

A semi-moot approval since this already merged by accident but :shipit:; thanks for doing this as this is a really, really nice cleanup that I'm sure was not fun doing the mechanical changes.

Base automatically changed from master to main March 3, 2021 23:53
@tmat tmat closed this Mar 5, 2021
@tmat
Copy link
Member Author

tmat commented Mar 5, 2021

Superseded by #51526

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants