-
Notifications
You must be signed in to change notification settings - Fork 9.8k
labels: faster Compare function when using -tags stringlabels #12451
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
Conversation
Extend tests too. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Instead of unpacking every individual string, we skip to the point where there is a difference. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
6db3be1
to
9ae580d
Compare
model/labels/labels_stringlabels.go
Outdated
if firstCharDifferent == len(shorter) { | ||
if len(shorter) == len(longer) { | ||
return 0 | ||
} else if firstCharDifferent == len(a.data) { |
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.
Nit, I think this is more readable:
} else if firstCharDifferent == len(a.data) { | |
} else if len(a.data) == len(shorter) { |
As this way the code reads as: if a is shorter...
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 read what I wrote as "if the first different character is at the end of a.data
".
I think yours requires to be thinking about longer and shorter, which I'm not at this point.
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 understand from if firstCharDifferent == len(shorter) {
that firstCharDifferent
actually lost its value, as it's not first char different now, but a signal saying: _ all common characters are equal, pending to check whether one string is longer than the other one._
Which now makes me think that it would be better to just write:
if firstCharDifferent == len(shorter) {
return compareLengths(a.data, b.data)
}
// ...
}
// ...
func compareLengths(a, b []string) int {
switch {
case len(a) == len(b):
return 0
case len(a) < len(b):
return -1
case len(b) > len(a):
return +1
}
}
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.
Both options are fine to me, but I agree with @colega naming of firstCharDifferent
is indeed misleading (premature - it might be just all equal characters. Perhaps keeping i
and not introducing new var might help here?
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.
func compareLengths(a, b string) int {
return len(a) - len(b)
}
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.
Oh great, so maybe then just:
if i == len(shorter) {
return len(a.data) - len(b.data)
}
firstCharDifferent := i
model/labels/labels_stringlabels.go
Outdated
i := 0 | ||
_ = longer[len(shorter)-1] // Get compiler to do bounds-check on longer just once here. | ||
for ; i < len(shorter); i++ { | ||
if shorter[i] != longer[i] { | ||
break | ||
} | ||
} |
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.
Since decodeSize
is very cheap, we can keep decoding sizes as we go, like:
i := 0 | |
_ = longer[len(shorter)-1] // Get compiler to do bounds-check on longer just once here. | |
for ; i < len(shorter); i++ { | |
if shorter[i] != longer[i] { | |
break | |
} | |
} | |
i := 0 | |
lastLblStart, nextLblStart := 0, 0 | |
_ = longer[len(shorter)-1] // Get compiler to do bounds-check on longer just once here. | |
for ; i < len(shorter); i++ { | |
if shorter[i] != longer[i] { | |
break | |
} | |
if i >= nextLblStart { | |
size, nextI := decodeSize(shorter, i) | |
nextLblStart = nextI + size | |
lastLblStart = i | |
} | |
} |
So then later we can just do i = lastLblStart
.
This gives pretty good performance impact:
goos: linux
goarch: amd64
pkg: github.com/prometheus/prometheus/model/labels
cpu: 11th Gen Intel(R) Core(TM) i7-11700K @ 3.60GHz
│ bryanagain │ tracknoloop │
│ sec/op │ sec/op vs base │
Labels_Compare/equal-16 25.59n ± 0% 26.48n ± 3% +3.46% (p=0.000 n=30+10)
Labels_Compare/not_equal-16 30.25n ± 1% 24.07n ± 3% -20.43% (p=0.000 n=30+10)
Labels_Compare/different_sizes-16 16.46n ± 1% 12.38n ± 8% -24.84% (p=0.000 n=30+10)
Labels_Compare/lots-16 47.77n ± 1% 42.97n ± 4% -10.06% (p=0.000 n=30+10)
geomean 27.94n 24.13n -13.63%
I've ran this benchmark multiple times and didn't get a consistent result (I was getting the equals
benchmark faster in my version, which doesn't make sense), however I also think that if we do this, we can remove the last for loop (as we already know which labels are different, no need to search for them).
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 would like to search for the first difference much faster, e.g. 8 bytes at a time using dwords, or even longer strides using SIMD. I made various attempts at this, e.g. 06f8bc7. I think it's faster than your version, although I don't really like the implementation in github.com/grailbio/base/simd.FirstUnequal8Unsafe
.
Putting an extra if-statement inside the tight loop does not seem attractive.
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.
If we're going unsafe, then why just not cast the 8 bytes to uint64?
That seems to give pretty good results:
goos: linux
goarch: amd64
pkg: github.com/prometheus/prometheus/model/labels
cpu: 11th Gen Intel(R) Core(TM) i7-11700K @ 3.60GHz
│ original │ unsafe_uint64 │
│ sec/op │ sec/op vs base │
Labels_Compare/equal-16 24.635n ± 0% 5.832n ± 2% -76.33% (p=0.000 n=10)
Labels_Compare/not_equal-16 30.33n ± 1% 14.37n ± 1% -52.63% (p=0.000 n=10)
Labels_Compare/different_sizes-16 16.125n ± 1% 5.353n ± 2% -66.80% (p=0.000 n=10)
Labels_Compare/lots-16 47.39n ± 0% 27.50n ± 2% -41.97% (p=0.000 n=10)
geomean 27.49n 10.54n -61.66%
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 included your latest code, and updated my benchmark results.
Now mostly going faster than the slice version.
Try each case both ways round. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
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.
Looks good 💪🏽 Thanks!
model/labels/labels_stringlabels.go
Outdated
if firstCharDifferent == len(shorter) { | ||
if len(shorter) == len(longer) { | ||
return 0 | ||
} else if firstCharDifferent == len(a.data) { |
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.
Both options are fine to me, but I agree with @colega naming of firstCharDifferent
is indeed misleading (premature - it might be just all equal characters. Perhaps keeping i
and not introducing new var might help here?
Compare even faster by casting the strings to uint64. Now the bounds-check optimisation doesn't make any difference, so we don't need to protect against zero-length strings on entry. Signed-off-by: Bryan Boreham <bjboreham@gmail.com> Co-authored-by: Oleg Zaytsev <mail@olegzaytsev.com>
Created an issue for changing `Compare` to `Less` - #12455 Changing to non-alphabetic sorting does not seem feasible, since previously-stored blocks are in alphabetic order. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
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.
🚀
@bwplotka a couple of things changed since you last approved so I'll let you take another look before merging. |
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.
…heus#12451) Instead of unpacking every individual string, we skip to the point where there is a difference, going 8 bytes at a time where possible. Add benchmark for Compare; extend tests too. --------- Signed-off-by: Bryan Boreham <bjboreham@gmail.com> Co-authored-by: Oleg Zaytsev <mail@olegzaytsev.com>
Instead of unpacking every individual string, we skip to the point where there is a difference.
Added a benchmark to illustrate.
[benchmark results updated 2023-06-15T1513]
Before/after this PR with
-tags stringlabels
:After this PR, without/with
-tags stringlabels
: