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

Move a specific regex to static variable #346

Merged
merged 5 commits into from
Apr 17, 2023

Conversation

another-rex
Copy link
Collaborator

Fixes #333

I measured the time taken to compile the regexp at startup and it's unnoticeable for these regexp, so I don't think we need the added complexity with sync.Once.

@G-Rath
Copy link
Collaborator

G-Rath commented Apr 14, 2023

imo the principle is what's important - as @spencerschrock said on #333 right now it happens that scorecard is scanning a large amount of yarn lockfiles relative to other lockfiles, so that's why this seems the slowest but that could very easily change and I know there's a huge regexp for requirements.txt which will have an impact.

So while I agree it's not necessarily worth the extra complexity of sync.Once for this case, I think it would be worth considering the principle on "should this regexp be global or not?" (to which sync.Once should mean no one has to actually debate over the benchmarks, as the rule can be just always use that and don't worry about it).

@another-rex
Copy link
Collaborator Author

@G-Rath Good point, I added a utility function that'll cache regex's globally and replaced most instances of regexp.MustCompile with it, so it should be relatively painless to use in the future as well. PTAL

@spencerschrock
Copy link
Contributor

I like the cached regex package approach, provides a simple replacement call without sacrificing readability or usability conventions that a caller would be responsible for following.

Wasn't aware of sync.Map but it seems like a good fit:

The Map type is optimized for two common use cases: (1) when the entry for a given key is only ever written once but read many times, as in caches that only grow

@G-Rath
Copy link
Collaborator

G-Rath commented Apr 16, 2023

fwiw it looks like this package provides exactly what you want, and doesn't have any other dependencies

@another-rex
Copy link
Collaborator Author

It does look like basically what is implemented here, but since this is just a couple of lines, I don't think it's necessary to pull in a dependency.

@another-rex another-rex merged commit 898751b into google:main Apr 17, 2023
@oliverchang
Copy link
Collaborator

It does look like basically what is implemented here, but since this is just a couple of lines, I don't think it's necessary to pull in a dependency.

+1. It's pretty trivial, and not worth adding this to our supply chain.

spencerschrock added a commit to spencerschrock/scorecard that referenced this pull request Apr 21, 2023
google/osv-scanner#346
Signed-off-by: Spencer Schrock <sschrock@google.com>
spencerschrock added a commit to spencerschrock/scorecard that referenced this pull request Apr 21, 2023
https: //github.com/google/osv-scanner/pull/346
Signed-off-by: Spencer Schrock <sschrock@google.com>
spencerschrock added a commit to spencerschrock/scorecard that referenced this pull request Apr 21, 2023
google/osv-scanner#346
Signed-off-by: Spencer Schrock <sschrock@google.com>
spencerschrock added a commit to ossf/scorecard that referenced this pull request Apr 24, 2023
)

* Recover from osv-scanner panics.

This allows us to give an inconclusive score instead of crashing.

Signed-off-by: Spencer Schrock <sschrock@google.com>

* Bump osv-scanner to include performance increase.

google/osv-scanner#346
Signed-off-by: Spencer Schrock <sschrock@google.com>

---------

Signed-off-by: Spencer Schrock <sschrock@google.com>
balteravishay pushed a commit to balteravishay/scorecard that referenced this pull request May 29, 2023
…sf#2896)

* Recover from osv-scanner panics.

This allows us to give an inconclusive score instead of crashing.

Signed-off-by: Spencer Schrock <sschrock@google.com>

* Bump osv-scanner to include performance increase.

google/osv-scanner#346
Signed-off-by: Spencer Schrock <sschrock@google.com>

---------

Signed-off-by: Spencer Schrock <sschrock@google.com>
Signed-off-by: Avishay <avishay.balter@gmail.com>
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.

Regexp compilation taking a long time
4 participants