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.

Sign up for GitHub

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

Jump to bottom

Microbenchmark perf regression in update benchmarks #27681

Closed
roji opened this issue Mar 21, 2022 · 2 comments
Closed

Microbenchmark perf regression in update benchmarks #27681

roji opened this issue Mar 21, 2022 · 2 comments
Assignees
@roji
Labels
area-perf area-save-changes closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone
7.0.0

Comments

@roji
Copy link
Member

roji commented Mar 21, 2022

Update, insert, delete:

image

I have some ideas...
@roji roji self-assigned this Mar 21, 2022
@ajcvickers ajcvickers added this to the 7.0.0 milestone Mar 22, 2022
@roji
Copy link
Member Author

roji commented Mar 24, 2022

Confirmed that the memory regression is because of #27584, where SingularModificationCommandBatch starts to add the second command to the batch, only to immediately roll it back. Adding a pre-check instead of the post-check brings us back from 10MB to 8MB.

The "operations per second" regression doesn't repro for me in the same way locally; I'll do the pre-check as above and follow up based on where the benchmark charts go after that.

Note that the update pipeline microbenchmarks need to be fixed - we have an InvocationCount of 1, presumably because we use [IterationSetup] and [IterationCleanup]; these should not be used for microbenchmarks. I'll look at fixing this after the product fix.

roji added a commit to roji/efcore that referenced this issue Mar 24, 2022
@roji
Reimplement MaxBatchSize as a pre-check
5b0552e 
For better perf on SQLite (dotnet#27681)

Also moves the handling of MaxBatchSize to
ReaderModificationCommandBatch and does some cleanup.
@roji roji mentioned this issue Mar 24, 2022
Merged
roji added a commit to roji/efcore that referenced this issue Mar 26, 2022
@roji
Reimplement MaxBatchSize as a pre-check
585ea4d 
For better perf on SQLite (dotnet#27681)

Also moves the handling of MaxBatchSize to
ReaderModificationCommandBatch and does some cleanup.
roji added a commit to roji/efcore that referenced this issue Mar 26, 2022
@roji
Reimplement MaxBatchSize as a pre-check
d75b8bb 
For better perf on SQLite (dotnet#27681)

Also moves the handling of MaxBatchSize to
ReaderModificationCommandBatch and does some cleanup.
ghost pushed a commit that referenced this issue Mar 26, 2022
@roji
Reimplement MaxBatchSize as a pre-check (#27696)
97b9376 
For better perf on SQLite (#27681)

Also moves the handling of MaxBatchSize to
ReaderModificationCommandBatch and does some cleanup.
@ajcvickers
Copy link
Contributor

Note from triage: main regression fixed.

@ajcvickers ajcvickers added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed propose-close labels Jul 7, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0, 7.0.0-preview7 Jul 7, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0-preview7, 7.0.0 Nov 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@roji roji

Labels
area-perf area-save-changes closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Milestone
7.0.0
Development

No branches or pull requests
2 participants
@ajcvickers @roji