-
Notifications
You must be signed in to change notification settings - Fork 4.5k
allow index update to change storage slot # #23311
allow index update to change storage slot # #23311
Conversation
180794b
to
d0f99c5
Compare
d0f99c5
to
f24fa07
Compare
94636a6
to
4bda786
Compare
4bda786
to
32dff59
Compare
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.
I think this looks good. It's hard for me to think about all the cases here, esp with add/deref. So like the comments below, maybe more tests/debug asserts would be valuable (if possible)?
Or maybe there's a way to refactor/simplify the loop in update_slot_list()
? It feels like there's some code duplication (but maybe not enough?).
Probably just getting lots of runtime on this is good.
867f4c7
to
cb06e8c
Compare
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.
I think this new impl looks good too. I think the for loop is simpler as well.
90c7bf5
to
41d629d
Compare
41d629d
to
18e72c4
Compare
Codecov Report
@@ Coverage Diff @@
## master #23311 +/- ##
========================================
Coverage 81.3% 81.4%
========================================
Files 573 573
Lines 156513 156610 +97
========================================
+ Hits 127383 127579 +196
+ Misses 29130 29031 -99 |
3e12b5a
to
8c08a09
Compare
Added some more tests. Verified snapshot startup time does not appear to be significantly impacted. Seems to be within the noise. |
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.
I think this looks good. I didn't follow all of the new test, though. Do you want me to spend more time on it? If yes, would you be willing to help me (jump on a call maybe)?
8c08a09
to
cbd56ae
Compare
Pull request has been modified.
cbd56ae
to
99412d6
Compare
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.
lgtm
Problem
this is used by ancient append vecs. Broken out here because of all the files and tests it touches.
Summary of Changes
Fixes #