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

Lazy init #153

Merged
merged 3 commits into from Sep 9, 2021
Merged

Lazy init #153

merged 3 commits into from Sep 9, 2021

Conversation

kolyshkin
Copy link
Collaborator

@kolyshkin kolyshkin commented Sep 7, 2021

With the following proggy:

package main

import 	_ "github.com/opencontainers/selinux/go-selinux"

func main() {}

we can easily measure the overhead of merely importing (not actually using) the package:

$ GODEBUG=inittrace=1 go run a.go 2>&1 | grep go-selinux
# github.com/opencontainers/selinux/go-selinux
init github.com/opencontainers/selinux/go-selinux @0.87 ms, 0.15 ms clock, 18328 bytes, 152 allocs

This PR tries to minimize that by:

  • removing regex usage;
  • initializing some stuff in a lazy (on demand) manner, rather than during initialization time

With these patches, we have

init github.com/opencontainers/selinux/go-selinux @0.53 ms, 0 ms clock, 160 bytes, 8 allocs

which eliminates most of the overhead.

Besides, modified readConfig and loadLabels show some performance and memory improvements as well:

[kir@kir-rhat go-selinux]$ benchstat before after
name          old time/op    new time/op    delta
ReadConfig-4    4.86µs ± 1%    3.46µs ± 2%  -28.82%  (p=0.002 n=6+6)
LoadLabels-4    12.7µs ± 3%     6.8µs ± 1%  -46.24%  (p=0.002 n=6+6)

name          old alloc/op   new alloc/op   delta
ReadConfig-4    5.08kB ± 0%    4.35kB ± 0%  -14.33%  (p=0.002 n=6+6)
LoadLabels-4    6.49kB ± 0%    6.10kB ± 0%   -6.07%  (p=0.000 n=6+5)

name          old allocs/op  new allocs/op  delta
ReadConfig-4      20.0 ± 0%       8.0 ± 0%  -60.00%  (p=0.002 n=6+6)
LoadLabels-4      45.0 ± 0%      45.0 ± 0%     ~     (all equal)

See individual commit messages for details.

Copy link
Collaborator

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM, nice catch

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.

left some questions / suggestions / thoughts 😅

go-selinux/selinux_linux.go Outdated Show resolved Hide resolved
go-selinux/selinux_linux.go Outdated Show resolved Hide resolved
go-selinux/selinux_linux.go Outdated Show resolved Hide resolved
go-selinux/selinux_linux.go Outdated Show resolved Hide resolved
go-selinux/selinux_linux.go Outdated Show resolved Hide resolved
go-selinux/selinux_linux.go Outdated Show resolved Hide resolved
go-selinux/selinux_linux.go Outdated Show resolved Hide resolved
@rhatdan
Copy link
Collaborator

rhatdan commented Sep 8, 2021

LGTM, but need to answer @thaJeztah feedback.

@kolyshkin kolyshkin force-pushed the lazy-init branch 3 times, most recently from 9bb3eeb to 9115ac7 Compare September 8, 2021 22:39
thaJeztah
thaJeztah previously approved these changes Sep 9, 2021
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

thanks! it's both cleaner and more performant, which is always nice!

@thaJeztah
Copy link
Member

@rhatdan ptal; ready to go if it still LGTY

@rhatdan
Copy link
Collaborator

rhatdan commented Sep 9, 2021

Rebase and we can merge, and I will cut a new release.

Initializing policyRoot involves reading and parsing a config file,
and it's not always used, so it's better to only init it when needed.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Perform lazy (on demand) init of labels, to reduce the impact
of importing the package.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Using a globally initialized regex incurs some overhead during package
initialization, which is noticeable for all other packages importing it.

Besides, there's no need to use regex to perform key=value split.

Rewrite both functions without using regex, and add benchmarks, which
show that the new code is faster:

name          old time/op    new time/op    delta
ReadConfig-4    4.86µs ± 1%    3.46µs ± 2%  -28.82%  (p=0.002 n=6+6)
LoadLabels-4    12.7µs ± 3%     6.8µs ± 1%  -46.24%  (p=0.002 n=6+6)

name          old alloc/op   new alloc/op   delta
ReadConfig-4    5.08kB ± 0%    4.35kB ± 0%  -14.33%  (p=0.002 n=6+6)
LoadLabels-4    6.49kB ± 0%    6.10kB ± 0%   -6.07%  (p=0.000 n=6+5)

name          old allocs/op  new allocs/op  delta
ReadConfig-4      20.0 ± 0%       8.0 ± 0%  -60.00%  (p=0.002 n=6+6)
LoadLabels-4      45.0 ± 0%      45.0 ± 0%     ~     (all equal)

Besides, the init overhead is eliminated:

Before:
init github.com/opencontainers/selinux/go-selinux @2.4 ms, 0.088 ms clock, 5016 bytes, 72 allocs

After:
init github.com/opencontainers/selinux/go-selinux @2.9 ms, 0.001 ms clock, 160 bytes, 8 allocs

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Collaborator Author

rebased

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.

still LGTM, thanks!

Copy link
Collaborator

@rhatdan rhatdan left a comment

Choose a reason for hiding this comment

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

LGTM

@rhatdan rhatdan merged commit 935cf7f into opencontainers:main Sep 9, 2021
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.

None yet

4 participants