Skip to content
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

meta: make NameEquals alloc-free #366

Merged
merged 2 commits into from
Mar 20, 2020

Conversation

quasilyte
Copy link
Contributor

Benchstat results:

name                 old time/op    new time/op    delta
NameEquals/1part-8     76.2ns ± 2%    15.3ns ± 2%   -79.94%  (p=0.008 n=5+5)
NameEquals/3parts-8     174ns ± 1%      30ns ± 1%   -82.74%  (p=0.008 n=5+5)

name                 old alloc/op   new alloc/op   delta
NameEquals/1part-8      16.0B ± 0%      0.0B       -100.00%  (p=0.008 n=5+5)
NameEquals/3parts-8     48.0B ± 0%      0.0B       -100.00%  (p=0.008 n=5+5)

name                 old allocs/op  new allocs/op  delta
NameEquals/1part-8       1.00 ± 0%      0.00       -100.00%  (p=0.008 n=5+5)
NameEquals/3parts-8      1.00 ± 0%      0.00       -100.00%  (p=0.008 n=5+5)

Refs #364

Signed-off-by: Iskander Sharipov quasilyte@gmail.com

name                 old time/op    new time/op    delta
NameEquals/1part-8     76.2ns ± 2%    15.3ns ± 2%   -79.94%  (p=0.008 n=5+5)
NameEquals/3parts-8     174ns ± 1%      30ns ± 1%   -82.74%  (p=0.008 n=5+5)

name                 old alloc/op   new alloc/op   delta
NameEquals/1part-8      16.0B ± 0%      0.0B       -100.00%  (p=0.008 n=5+5)
NameEquals/3parts-8     48.0B ± 0%      0.0B       -100.00%  (p=0.008 n=5+5)

name                 old allocs/op  new allocs/op  delta
NameEquals/1part-8       1.00 ± 0%      0.00       -100.00%  (p=0.008 n=5+5)
NameEquals/3parts-8      1.00 ± 0%      0.00       -100.00%  (p=0.008 n=5+5)

Refs #364

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>
@quasilyte quasilyte force-pushed the quasilyte/performance/optimize_name_equals branch from d45856a to 6324659 Compare March 20, 2020 11:20
p, ok := part.(*name.NamePart)
if !ok {
part := part.(*name.NamePart)
if !strings.HasPrefix(rest, part.Value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is slightly incorrect. I think we also need to check two things:

  1. That there is a character \ after part.Value ends if it's not the final part (essentially that the prefix is part.Value + "\\").
  2. If it is the final part we need to check that there are no more characters in the string.

Otherwise the following would be equal:

a\b = a\bb because we only check the prefixes
a b\ = a\b because we don't check where is \ symbol

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks!

I believe if we handle the last part separately with "==" instead of "hasPrefix", all issues go away.

I've added test cases that check whether the mentioned scenarios lead to expected results.

1 a\b = a\bb because we only check the prefixes

Now we check the last part with ==, should be fixed

  1. a b\ = a\b because we don't check where is \ symbol

This will fail in old code as well, since the second part will be checked as
HasPrefix("b", ""), which is false. In updated code it shouldn't be an issue as well, as "b"!="".

  1. That there is a character \ after part.Value ends if it's not the final part (essentially that the prefix is part.Value + "\").

I can't make up an example that will pass NameEquals with something except \ as a separator. It will cause either different numbers of parts (we have an early check for it) or difference during the next part comparison. If you have an example, I can add it to tests and then it will be easier to fix it.

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>
@quasilyte quasilyte merged commit 704852c into master Mar 20, 2020
@quasilyte quasilyte deleted the quasilyte/performance/optimize_name_equals branch March 20, 2020 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants