CNDB-15669: Implement mayHaveDeletionBranch#6
Conversation
…lag in trie cursors. This metadata flag allows skipping content lookups for structural nodes and is correctly propagated and unioned across all cursor implementations.
… using MAY_HAVE_DELETION_BRANCH_BIT flag Introduces Cursor.MAY_HAVE_DELETION_BRANCH_BIT flag to optimize read paths in deletion-aware tries. By setting this flag on cursors only when a deletion branch is present at the node, we bypass checking for deletion branches in merges, traversals, and lookups when the flag is not set.
| RangeCursor<D> deletionsBranch = c1.deletionBranchCursor(direction()); | ||
| if (deletionsBranch != null) | ||
| addCursor(deletionsBranch); | ||
| if ((c1.encodedPosition() & MAY_HAVE_DELETION_BRANCH_BIT) != 0) |
There was a problem hiding this comment.
Can't we use encodedPosition here instead of calling the method?
| rc = dac.deletionBranchCursor(Direction.FORWARD); | ||
| if (rc != null) | ||
| break; | ||
| if ((dac.encodedPosition() & Cursor.MAY_HAVE_DELETION_BRANCH_BIT) != 0) |
There was a problem hiding this comment.
Take advantage of currentPosition here (the whole point of the flag is to avoid the method call by using information we already have).
| RangeCursor<D> deletionsBranch = cursor.deletionBranchCursor(direction); | ||
| if (deletionsBranch != null) | ||
| collectedDeletionBranches.add(deletionsBranch); | ||
| if ((cursor.encodedPosition() & Cursor.MAY_HAVE_DELETION_BRANCH_BIT) != 0) |
There was a problem hiding this comment.
Could you measure if this is actually beneficial? Calling encodedPosition is itself a megamorphic call, so we might be making things worse here.
| @Override | ||
| long collectFlagsMask() | ||
| { | ||
| return Cursor.MAY_HAVE_CONTENT_BIT | Cursor.MAY_HAVE_DELETION_BRANCH_BIT; |
There was a problem hiding this comment.
If deletionsAtFixedPoints is false and we are already under a deletion, we should not collect or present the MAY_HAVE_DELETION_BRANCH_BIT of sources.
| // Otherwise we need to get the deletions from all sources to track and apply them. | ||
| if (branchHasMultipleSources()) | ||
| { | ||
| RangeCursor<D> deletions = deletionsAtFixedPoints ? makeRelevantDeletionsFixedPoints() |
There was a problem hiding this comment.
We can take advantage of the collected position's flags before calling these.
| RangeCursor<D> deletionBranch = c.deletionBranchCursor(Direction.FORWARD); | ||
| if (deletionBranch != null) | ||
| return tailTrieSeparately(ByteSource.duplicatable(bytes), c, deletionBranch, includeCoveringDeletions); | ||
| if ((c.encodedPosition() & Cursor.MAY_HAVE_DELETION_BRANCH_BIT) != 0) |
There was a problem hiding this comment.
Let's refactor this to keep track of the position returned by skipTo and avoid the calls to encodedPosition.
| T processPrefix(int node, int depth, int transition) | ||
| { | ||
| T content = super.processPrefix(node, depth, transition); | ||
| if (trie.getAlternateBranch(node) != NONE) |
There was a problem hiding this comment.
Since we already know this is a prefix node, let's read the trie directly (trie.getIntVolatile(node + PREFIX_ALTERNATE_OFFSET)).
| assert !c2.hasDeletions(); | ||
| } | ||
|
|
||
| if (atC1 && atC2 && // otherwise even if there is deletion, the other cursor is ahead of it and can't be affected |
There was a problem hiding this comment.
We can check the flag in encodedPosition before calling these.
Also, this should filter out the deletion branch bit if we are under a deletion in non-fixed-point mode (i.e. !deletionsAtFixedPoints && deletionBranchDepth != -1).
| // deletion-tree branch of the other to make sure that we merge any higher-depth deletion branch with it. | ||
| RangeCursor<D> b1 = c1.deletionBranchCursor(direction); | ||
| RangeCursor<E> b2 = c2.deletionBranchCursor(direction); | ||
| RangeCursor<D> b1 = (c1.encodedPosition() & MAY_HAVE_DELETION_BRANCH_BIT) != 0 ? c1.deletionBranchCursor(direction) : null; |
There was a problem hiding this comment.
As in the other pull request, we may benefit from a static Cursor method.
| public long advanceMultiple(TransitionsReceiver receiver) | ||
| { | ||
| long pos = super.advanceMultiple(receiver); | ||
| if (atEnd()) |
There was a problem hiding this comment.
Aren't we always atEnd() after advanceMultiple?
2 participants
Branch created from #4