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

libct/specconv: checkPropertyName speedup #3365

Merged
merged 2 commits into from
Feb 17, 2022

Conversation

kolyshkin
Copy link
Contributor

See commits for details.

Mostly done because the comment was misleading, not in line with what the code did.

Fix the code, so now the comment is fine.

Commit 643f8a2 renamed isValidName to checkPropertyName, but fell
short of renaming its test and benchmark. Fix that.

Fixes: 643f8a2
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 029b73c replaced a regular expression with code checking the
characters. Despite what the comment said about ASCII, the check was
performed rune by rune, not byte by byte.

Note the check was still correct, basically comparing int32's, but the
byte by byte way is a tad faster and more straightforward. The change
also fixes the issue of a misleading comment.

Benchmark before/after shows a modest improvement:

name                 old time/op    new time/op    delta
CheckPropertyName-4     164ns ± 2%     123ns ± 2%  -24.73%  (p=0.029 n=4+4)

name                 old alloc/op   new alloc/op   delta
CheckPropertyName-4     96.0B ± 0%     64.0B ± 0%  -33.33%  (p=0.029 n=4+4)

name                 old allocs/op  new allocs/op  delta
CheckPropertyName-4      6.00 ± 0%      4.00 ± 0%  -33.33%  (p=0.029 n=4+4)

Fixes: 029b73c
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin kolyshkin changed the title checkPropertyName: speedup libct/specconv: checkPropertyName speedup Feb 1, 2022
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

libcontainer/specconv/spec_linux.go Show resolved Hide resolved
@kolyshkin
Copy link
Contributor Author

@opencontainers/runc-maintainers PTAL (this is very easy to review)

@AkihiroSuda AkihiroSuda merged commit 2436322 into opencontainers:main Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants