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

GH-2797: Lazy init for ValidationState hash maps #2798

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Ostrzyciel
Copy link
Contributor

GitHub issue resolved #2797

Pull request Description:

This is a conservative solution to #2797 – it does not change any behavior from the outside perspective, and only changes the initialization of these two hashmaps to be lazy.

This should improve RDF parsing performance.

I'm marking "tests are included" because I don't see anything to test here that is not already covered by existing tests.


  • Tests are included.
  • Documentation change and updates are provided for the Apache Jena website
  • Commits have been squashed to remove intermediate development commit messages.
  • Key commit messages start with the issue number (GH-xxxx)

By submitting this pull request, I acknowledge that I am making a contribution to the Apache Software Foundation under the terms and conditions of the Contributor's Agreement.


See the Apache Jena "Contributing" guide.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Doesn't this allow for possible multithreading issues though? While with the final map that's mitigated... maybe looking at other data structures/map implementations could solve it in a different way (there's a comment about ligher impl...)

@Ostrzyciel
Copy link
Contributor Author

Doesn't this allow for possible multithreading issues though? While with the final map that's mitigated... maybe looking at other data structures/map implementations could solve it in a different way (there's a comment about ligher impl...)

The class is not thread-safe anyway, because HashMap is not thread-safe, so... it should be OK?

We would have to worry about it if we had a shared instance of ValidationState across multiple parsers, but we don't. Long-term, I think this would be a good idea, but for that to happen I think most of this random xerces bloat would have to be removed first, as it's mostly dead code anyway.

@Ostrzyciel
Copy link
Contributor Author

...I mean, I can make this thread-safe, but the real question is "what for" 😆

@arne-bdt
Copy link
Contributor

...I mean, I can make this thread-safe, but the real question is "what for" 😆

Maybe add a comment to the javadoc header of the class with:
* This class is not thread safe.

@Ostrzyciel
Copy link
Contributor Author

...I mean, I can make this thread-safe, but the real question is "what for" 😆

Maybe add a comment to the javadoc header of the class with: * This class is not thread safe.

Done.

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.

Unneeded HashMap allocations in ValidationState
3 participants