-
Notifications
You must be signed in to change notification settings - Fork 40
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
bundle: Load bundle once #1136
bundle: Load bundle once #1136
Conversation
@@ -102,8 +100,8 @@ func (l Linter) WithInputModules(input *rules.Input) Linter { | |||
} | |||
|
|||
// WithAddedBundle adds a bundle of rules and data to include in evaluation. | |||
func (l Linter) WithAddedBundle(b bundle.Bundle) Linter { | |||
l.ruleBundles = append(l.ruleBundles, &b) | |||
func (l Linter) WithAddedBundle(b *bundle.Bundle) Linter { |
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.
This is an API change. We can do it without this, but it feels right to do it given how this function is used.
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.
This is not used in the Rego playground or in any public code on https://sourcegraph.com/search?q=context:global+WithAddedBundle&patternType=keyword&sm=0
4746212
to
e0f22d9
Compare
bundle/bundle.go
Outdated
loadedBundleOnce sync.Once | ||
) | ||
|
||
// LoadedBundle returns the loaded Regal bundle for this version of Regal. |
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 take it you'd like to keep it being lazily loaded? Because if it was going to be used anyways, often times sync.Once etc can be replaced with a package var, like
var loadedBundle = io.MustLoadRegalBundleFS(Bundle)
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.
Perhaps we should just do this 👍 I was remembering a discussion about how inits in the ls implementation were slowing down the linting cmd, but that's not the case here.
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 in 9c2e409
Previously, loading of the bundle happened many times, each time it takes around 20ms. This improves the performance of the language server by loading the bundle once and reusing it since the bundle is never changed once a binary is running. Signed-off-by: Charlie Egan <charlie@styra.com>
Follows PR review comment Signed-off-by: Charlie Egan <charlie@styra.com>
e0f22d9
to
9c2e409
Compare
Thanks Stephan! |
* bundle: Load bundle once Previously, loading of the bundle happened many times, each time it takes around 20ms. This improves the performance of the language server by loading the bundle once and reusing it since the bundle is never changed once a binary is running. Signed-off-by: Charlie Egan <charlie@styra.com> * bundle: use pkg var Follows PR review comment Signed-off-by: Charlie Egan <charlie@styra.com> --------- Signed-off-by: Charlie Egan <charlie@styra.com>
* bundle: Load bundle once Previously, loading of the bundle happened many times, each time it takes around 20ms. This improves the performance of the language server by loading the bundle once and reusing it since the bundle is never changed once a binary is running. Signed-off-by: Charlie Egan <charlie@styra.com> * bundle: use pkg var Follows PR review comment Signed-off-by: Charlie Egan <charlie@styra.com> --------- Signed-off-by: Charlie Egan <charlie@styra.com>
Previously, loading of the bundle happened many times, each time it takes around 20ms. This improves the performance of the language server by loading the bundle once and reusing it since the bundle is never changed once a binary is running.
before
Screen.Recording.2024-09-25.at.10.19.12.mov
after
Screen.Recording.2024-09-25.at.10.20.29.mov