-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
optimize migrator's executeWriteFuncs #1124
base: master
Are you sure you want to change the base?
Conversation
Change-Id: I54619b17d43ae4b26dad0977b78c611976a06de0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting idea. I left a few review comments, thanks @debug-LiXiwen! 🙏
Change-Id: I89c4ec04fc28efcca6fc911eca51bfbcbff924b3
@debug-LiXiwen thanks for the PR changes, it is passing CI now 👍 Now that the code looks good, I'm curious if you could explain what situations this optimizes? If I understand correctly, this is the scenario this optimizes:
Did I get this right and are there more scenarios? The "window" for these events to appear above is extremely small. How likely is it that this scenario happens? Shouldn't the applier queue be empty by the time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more requested change I noticed now
|
That's a good point, the |
Change-Id: Ic1c90803c4fe8a43d1192a65ca2d331364a50eab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution @debug-LiXiwen! 🙇
I think relying on
I think |
Agreed, I think it's very unlikely this would improve performance for most workloads
I would be on board with this 👍. It's is incredibly unlikely there are new events to apply immediately after the |
A Pull Request should be associated with an Issue.
Description