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

Reduce SaveChanges roundtrips #27713

Merged
1 commit merged into from
Apr 26, 2022
Merged

Reduce SaveChanges roundtrips #27713

1 commit merged into from
Apr 26, 2022

Conversation

roji
Copy link
Member

@roji roji commented Mar 27, 2022

This is a draft for allowing batching in more cases by only applying a batch boundary for foreign keys where a principal key property is database-generated. There's still work and failing tests, but it seems like a good point to get feedback on the general approach.

  • Multigraph edges now have an additional flag - RequiresBatchBoundary.
  • The batching topological sort continuously adds to the same batch, unless it sees an edge with RequiresBatchBoundary=true pointing to a vertex that's in the current batch.
  • Since we now mix commands with dependencies in the same batch (as long as the dependency doesn't require a batch boundary), we can no longer do "secondary" sorting within the batch (ModificationCommandComparer). For now that's removed, I'm not sure of the exact importance of this.

See tests BatchCommands_does_not_create_batch_without_principal_key_database_generation, BatchCommands_creates_batch_with_principal_key_database_generation, Delete_Add_with_same_entity_types.

Closes #20664

@roji roji requested a review from AndriySvyryd March 27, 2022 12:05
@roji roji changed the title WIP on ultimate roundtrip reduction Reduce SaveChanges roundtrips Mar 27, 2022
@AndriySvyryd
Copy link
Member

The secondary sort is very important to prevent deadlocks in concurrency scenarios. This might also mean that the same-table edges would also need to be batch-breaking.

I was thinking that BatchingTopologicalSort could return IReadOnlyList<(List<TVertex>, bool)> where the bool would indicate a hard boundary.

@roji
Copy link
Member Author

roji commented Mar 29, 2022

Can you give a concrete example for the secondary sort, so i know what it's about?

Sure, we could expose the extra bool flag batch boundary. Another idea is to do the secondary sort inside the Multigraph, before appending the new roots to the batch.

@AndriySvyryd
Copy link
Member

#6666
#15180
#14371

@roji roji force-pushed the UpdatePipeline/BatchMoar branch 2 times, most recently from 8c99af0 to 7751ac1 Compare April 21, 2022 19:08
@roji roji force-pushed the UpdatePipeline/BatchMoar branch 2 times, most recently from 609fe1e to d12b383 Compare April 22, 2022 12:01
@roji
Copy link
Member Author

roji commented Apr 22, 2022

New version up @AndriySvyryd. Some additional comments:

  • Integrated secondary sorting into Multigraph. This simplifies things, but is also necessary since we need secondary sorting for non-batching topological sort (ModelExtensions.GetEntityTypesInHierarchicalOrder), where this cannot be done externally.
  • Implemented TopologicalSort over BatchingTopologicalSort, mainly since supporting secondary sorting requires lots of the same functionality (and DRY). The case where TopologicalSort is used without a secondary sort comparer will be slower, but it doesn't seem we use that in any critical place.
  • Note that the batch boundary logic is implemented inside Multigraph; it may be better to have it in ModificationCommandBatch instead (e.g. to allow providers to tweak it, and to simplify Multigraph). However, this could get complicated, and in order to do this efficiently we'd need to shove various things into ModificationCommand itself.

For the record, this improves the followings scenarios:

  • When inserting principal and dependent(s) and the principal key isn't database-generated. This is notably the case for GUID and HiLo, but also for TPT.
  • Deleting and updating to the same unique value
  • Delete and insert into the same table

@roji roji marked this pull request as ready for review April 22, 2022 12:05
@roji roji force-pushed the UpdatePipeline/BatchMoar branch from d12b383 to 156cbcc Compare April 26, 2022 09:23
@ghost
Copy link

ghost commented Apr 26, 2022

Hello @roji!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost
Copy link

ghost commented Apr 26, 2022

Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:

  1. Azure Pipelines

These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check.

Give feedback on this
From the bot dev team

We've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments.

Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin.

@ghost ghost merged commit 8236cb9 into dotnet:main Apr 26, 2022
@roji roji deleted the UpdatePipeline/BatchMoar branch April 26, 2022 15:18
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove unnecessary extra batches in SaveChanges
2 participants