Make upsert inner segment update atomic #7844
Conversation
| _result = _partialUpsertHandler.merge(previousRecord, record); | ||
| } | ||
| assert currentSegment.getValidDocIds() != null; |
There was a problem hiding this comment.
why do we need to move this up? can you add some comment?
There was a problem hiding this comment.
assert is used to suppress the IDE warning on potential NPE without adding overhead. It generally means it is not possible to have null validDocIds. I don't think it is worth commenting though
There was a problem hiding this comment.
I meant segment.getValidDocIds() is called earlier
There was a problem hiding this comment.
Assert statements generally do add some overhead because they add about 10 bytes of bytecode each, which interferes with hotspot’s bytecode size based inlining heuristics and may result in a method containing asserts not being inlined. I absolutely wouldn’t suggest removing it, but it’s important to know they aren’t free.
There was a problem hiding this comment.
Also, don’t be concerned about adding null checks, they aren’t compiled the way you see them - they get speculatively optimised away based on profiling data collected in lower tiers. So this is not more efficient than a null check which never fails, it’s exactly as efficient but with more bytecode weight. This is a good related read on the topic (it’s about implicit null checks which raise NPEs, but the same mechanism applies to branches not taken): https://shipilev.net/jvm/anatomy-quarks/25-implicit-null-checks/
There was a problem hiding this comment.
@yupeng9 That shouldn't matter as it just reads a member variable from the segment into a local variable. I simply extracted that common logic ahead to avoid making multiple assertions.
@richardstartin Great article, thanks for sharing. In that case should we prefer Objects.requireNonNull(currentSegment.getValidDocIds()).remove(currentDocId); as IDE suggested to suppress the warning?
There was a problem hiding this comment.
@Jackie-Jiang what about just a null check? If it really can't be null then it doesn't change the logic. Also, if it really can't be null, why not annotate the method @Nonnull?
There was a problem hiding this comment.
@richardstartin The method can return null for non-upsert case, but in the current context it can never be null. Objects.requireNonNull() is basically the concise version null check without putting the if/else block
Codecov Report
@@ Coverage Diff @@
## master #7844 +/- ##
============================================
- Coverage 71.68% 65.28% -6.41%
- Complexity 4089 4091 +2
============================================
Files 1579 1535 -44
Lines 80819 79223 -1596
Branches 12017 11865 -152
============================================
- Hits 57932 51717 -6215
- Misses 18982 23833 +4851
+ Partials 3905 3673 -232
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
When updating the upsert metadata, if the update (invalidate an old record, and validate a new record) applies to the same segment, make the update atomic to reduce the temporary inconsistency.
When updating the upsert metadata, if the update (invalidate an old record, and validate a new record) applies to the same segment, make the update atomic to reduce the temporary inconsistency.