-
Notifications
You must be signed in to change notification settings - Fork 56
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
perf: speedup hermit exec #431
Conversation
Add concurrency to Loader.All by parsing each hcl file in a separate goroutine. This results in an execution time that is about 3x faster on my machine. This change is most visible when Hermit must parse all packages during `hermit exec`.
Previously, when `hermit exec` was run on a package with a `requires` field, it would parse every known package to gather a list of candidates to fulfill the requirement. In the vast majority of cases, the user will already have the dependent package installed, so optimize by first evaluating only the installed packages. This change has no appreciable effect on the negative path where we *do* end up parsing all known packages. The result is a speedup by about a factor of 5 to 10 on my machine.
manifest/loader.go
Outdated
name string | ||
} | ||
mftC := make(chan result) | ||
wg := sync.WaitGroup{} |
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 like the idea, but I think it would be a good idea to throttle this a bit. There are ~500 packages currently, so this could generate a lot of I/O and garbage in a very short period of time. Maybe use an errgroup with SetLimit(), or a "semaphore" channel to limit to NumCPU() * 4 or something?
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.
Fixed. My understanding is that in order to actually throttle anything, we'd need to set the limit to below NumCPU. I arbitrarily chose max(3, runtime.NumCPU() / 4)
since this seemed to reduce the max CPU% utilization a lot on both my beefy and meh machines while still having about the same speedup.
There are currently a few hundred packages, and we don't want Hermit to cause a resource usage spike, so throttle concurrent package loading to a fraction of the number of threads available.
Excellent! |
For posterity: the majority of the remaining time in @alecthomas It looks like the net effect of parsing all packages is that we get all environment variables from all packages when we exec the process. Are you aware of any cases where that's particularly useful as opposed to only parsing the requested package? I can see it being potentially necessary for |
Consists of two changes.
Ran with
-race
to ensure I got the concurrency right.Example: running maven without the change, with the change, and without hermit (as a control)
Without the change:
With the change:
Without hermit:
Fixes: #396