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

Filters scans service loader over and over #1348

Open
1 task done
aaime opened this issue May 23, 2024 · 3 comments
Open
1 task done

Filters scans service loader over and over #1348

aaime opened this issue May 23, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@aaime
Copy link
Contributor

aaime commented May 23, 2024

Versions impacted by the bug

5.x

What went wrong?

While profiling the netcdf library I've noticed that the Filters.getFilter method shows up in the profile as a time sync, in particular, the bits calling onto ServiceLoader.

Suggestions: at a minimum, call onto the service loader only once in the method. For bonus points, perform the classpath scan only once in a static initializer, save in an immutable list, and just reuse over and over the results.
While it's possible to dynamically add class loaders at runtime, potentially fetching from jars that were not part of the classpath defined at JVM startup, this seems like a minority case, and maybe a "reload" method could be added to care for that subset of case.

Relevant stack trace

No response

Relevant log messages

No response

If you have an example file that you can share, please attach it to this issue.

If so, may we include it in our test datasets to help ensure the bug does not return once fixed?
Note: the test datasets are publicly accessible without restriction.

Yes

Code of Conduct

  • I agree to follow the UCAR/Unidata Code of Conduct
@aaime aaime added the bug Something isn't working label May 23, 2024
@aaime
Copy link
Contributor Author

aaime commented May 23, 2024

Oh, BTW, if you agree with any of the suggestions, I'm happy to follow up with a PR.

@haileyajohnson
Copy link
Member

Thanks for the insight! We'd love a PR if you're able.

@aaime
Copy link
Contributor Author

aaime commented May 23, 2024

Would you be ok with the "load list once on class initialization, and expose a reset/reload method for the oddball cases where the classpath is modified at runtime"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants