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

No heap allocations #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

erikdubbelboer
Copy link
Contributor

Don't heap allocate to lowercase strings.

This only works for ASCII characters but since we're only matching against ASCII strings it doesn't matter what happens to non-ASCII characters.

This turns out to be slightly faster as well:

benchmark                         old ns/op     new ns/op     delta
BenchmarkAgentSurfer-8            3605          3371          -6.49%
BenchmarkAgentSurferReuse-8       3591          3333          -7.18%
BenchmarkEvalSystem-8             2076          2046          -1.45%
BenchmarkEvalBrowserName-8        1257          1261          +0.32%
BenchmarkEvalBrowserVersion-8     69.3          69.5          +0.29%
BenchmarkEvalDevice-8             756           750           -0.79%
BenchmarkParseChromeMac-8         1237          1140          -7.84%
BenchmarkParseChromeWin-8         1149          1037          -9.75%
BenchmarkParseChromeAndroid-8     3753          3540          -5.68%
BenchmarkParseSafariMac-8         2012          1888          -6.16%
BenchmarkParseSafariiPad-8        2031          1892          -6.84%

Don't heap allocate to lowercase strings. This only works for ASCII
characters but since we're only matching against ASCII strings it
doesn't matter what happens to non-ASCII characters.

This turns out to be slightly faster as well:
benchmark                         old ns/op     new ns/op     delta
BenchmarkAgentSurfer-8            3605          3371          -6.49%
BenchmarkAgentSurferReuse-8       3591          3333          -7.18%
BenchmarkEvalSystem-8             2076          2046          -1.45%
BenchmarkEvalBrowserName-8        1257          1261          +0.32%
BenchmarkEvalBrowserVersion-8     69.3          69.5          +0.29%
BenchmarkEvalDevice-8             756           750           -0.79%
BenchmarkParseChromeMac-8         1237          1140          -7.84%
BenchmarkParseChromeWin-8         1149          1037          -9.75%
BenchmarkParseChromeAndroid-8     3753          3540          -5.68%
BenchmarkParseSafariMac-8         2012          1888          -6.16%
BenchmarkParseSafariiPad-8        2031          1892          -6.84%
@erikdubbelboer
Copy link
Contributor Author

@ryanslade any reason this isn't merged yet?

@ryanslade
Copy link
Contributor

Thanks for this, however I'd rather not introduce the use of the unsafe package.

Also, there is a chance that a user agent contains non ascii characters which this change ignores.

@erikdubbelboer
Copy link
Contributor Author

Yes the user agent might contains non ascii characters but those aren't used in any of the matching so it wouldn't matter if they are lower cased or not.

What's the reason for not wanting to introduce the use of the unsafe package?

@dzbarsky
Copy link

@erikdubbelboer It seems like you can use strings.Builder instead of an []byte and then you don't need the unsafe string/[]byte conversion. Also, you can create a parser struct that has the strings.Builder on it to reuse the allocation, then you wouldn't need the sync.Pool.

@erikdubbelboer
Copy link
Contributor Author

This is about the string that is lowercased. strings.Builder has nothing to do with it. Besides strings.Builder always causes an allocation which is what this pull request is trying to avoid.

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.

3 participants