Open
Conversation
Member
Author
|
@seqbenchbot up main search-logbench |
Contributor
🔴 Performance DegradationSome benchmarks have degraded compared to the previous run. Show table
|
cheb0
changed the title
cheb0
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #358 +/- ##
==========================================
- Coverage 71.51% 71.44% -0.08%
==========================================
Files 205 205
Lines 14872 14921 +49
==========================================
+ Hits 10636 10660 +24
- Misses 3468 3486 +18
- Partials 768 775 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cheb0
changed the title
cheb0
force-pushed
the
0-fast-cmp-xor
branch
from
b47b8ee to
ea92b83
Compare
cheb0
force-pushed
the
0-fast-cmp-xor
branch
from
ea92b83 to
7de3263
Compare
cheb0
changed the title
Contributor
🔴 Performance DegradationSome benchmarks have degraded compared to the previous run. Show table
|
eguguchkin
requested review from
dkharms,
eguguchkin and
forshev
and removed request for
dkharms
forshev
approved these changes
Mar 2, 2026
| withAggQuery(processor.AggQuery{GroupBy: aggField("level")})), | ||
| []map[string]uint64{ | ||
| {"gateway": 3, "proxy": 2, "scheduler": 1}, | ||
| {gateway: 3, proxy: 2, "scheduler": 1}, |
Contributor
There was a problem hiding this comment.
nit: use const scheduler here as well
| } | ||
|
|
||
| func (c LID) Eq(other LID) bool { | ||
| return c.lid == other.lid |
Contributor
There was a problem hiding this comment.
nit: is it possible scenario when user of this method compares asc lid and desc lid? at first glance looks like no. however, do we need to take order into account here and check mask?
for example
descLid := NewLIDOrderDesc(5)
ascLid := NewLIDOrderAsc(math.MaxUint32 - 5)
descLid.Eq(ascLid) // produces true now| } | ||
|
|
||
| // Less compares two values. It also does an implicit null check, since we store math.MaxUint32 for null values. | ||
| // Which means if we call x.Less(y), then we now for sure that x is not null. Therefore, this Less call can work |
Contributor
There was a problem hiding this comment.
nit: then we know for sure?
| n.readLeft() | ||
| } | ||
| for n.hasLeft && n.hasRight && n.less(n.rightID, n.leftID) { | ||
| for !n.rightID.IsNull() && n.rightID.Less(n.leftID) { |
Contributor
There was a problem hiding this comment.
nit: should it be !n.leftID.IsNull() && n.rightID.Less(n.leftID) so we check both ids for null?
| // For reverse order LID is inverted as follows: "MaxUint32 - LID" formula using XOR mask. Terminal LID value is 0 instead | ||
| // of MaxUint32 in reverse order, but 0 is XORed to MaxUint32. Which means, null value will always have lid field set to | ||
| // 0xFFFFFFFF (math.MaxUint32) regardless of reverse (order) flag. | ||
| type LID struct { |
Contributor
There was a problem hiding this comment.
nit: maybe ComparableLID? a bit longer but gives more context imo
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a zero-cost comparison for LIDs in iterators, unblocks NextGEQ (need fast comparisons and
minfunction for NextGEQ to be effective)Details
Instead of calling the
n.lesscomparator, we introduce a new type which hides all comparison logic. It's a structnode.LID(name can be changed).Struct size is 64 bits. For default order (desc) we keep two fields:
lid: LID (32 bits), XOR mask: 0 (32 bits)For reverse (order asc) we keep:
lid: LID ^ 0xFFFFFFFFF (32 bits), mask: 0xFFFFFFFFF (32 bits)To unpack a LID we just do
lid ^ mask.This makes masked LID value always go in ascending way (both orders). i.e. for default order (docs sort desc) LIDs remain unchanged, but for reverse odder (docs sort asc) higher LID values are transformed to lower values in lid field in
LIDstruct. Therefore, nodes no longer need to maintainreverseflag and execute same code for both orders. We can also effieciently compare values via ordinary<. We can also support efficientmin/maxfunction.Leaf nodes (
IteratorDesc/Asc,staticAsc/Desc) return terminal values. For default order we return the last LID asMaxUint32, for reverse order it's0. Inversion makes both values equal (first 32 bits), we can always tell if it's null value just by comparing withMaxUint32.Because null values are represented as
MaxUint32masked lid, we can eliminate unneeded if checks. Previously we had:n.hasLeft && n.less(n.leftID, n.rightID)Now we can just:
leftID.Less(rightID)Because null values are represented as
MaxUint32, this if check will never pass ifleftIDis null.The encoded
LIDvalue is yielded directly from eval tree (search) and passed right to agg tree, where it can also be effectively compared to different values.Measurements
Fractions taken from prod:
service:large_service, count by k8_pod (hot, data cached)master: ~470 ms
fast cmp: ~420 ms
service:small_service, count by k8_pod (hot, data cached)master: ~400 ms
fast cmp: ~330ms