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

Suggest a rework for DictionaryData in java port. #1211

Open
XenoAmess opened this issue Oct 21, 2024 · 3 comments
Open

Suggest a rework for DictionaryData in java port. #1211

XenoAmess opened this issue Oct 21, 2024 · 3 comments

Comments

@XenoAmess
Copy link

just, don't make a 123KB class if not have to. that is not a good way.
a better practice would be:

  1. remove the reflection in Dictionary.
  2. rework DictionaryData to load from resources file.
  3. move the load part from static part to method invocation part, and make a 2-step-sync to the loading part.

would be glad to impl it if you would arrange time for the code review.

@eustas
Copy link
Collaborator

eustas commented Nov 19, 2024

This design has a long history and meets some non-obvious requirements:

  • make sure it is zero-cost even at class tree is population time
  • do not rely on resource loading
  • allow dropping dictionary at all

With current implementation, reflection is not triggered if embedder has already invoked Dictionary.setData; in such case provided data could have been obtained by different means, e.g. loaded from resources.

If the main problem is class size, then you can compile/link with alternative DictionaryData implementation that loads from resources.

Please provide more context on the problem you are facing, and we could think how to improve the codebase.

@XenoAmess
Copy link
Author

This design has a long history and meets some non-obvious requirements:

  • make sure it is zero-cost even at class tree is population time
  • do not rely on resource loading
  • allow dropping dictionary at all

With current implementation, reflection is not triggered if embedder has already invoked Dictionary.setData; in such case provided data could have been obtained by different means, e.g. loaded from resources.

If the main problem is class size, then you can compile/link with alternative DictionaryData implementation that loads from resources.

Please provide more context on the problem you are facing, and we could think how to improve the codebase.

I'm using graalvm native image, so of course as small class as possible and as few reflections as possible means something to me....

But the real reason if I remember it correctly why I give up with brotli last month is I cannot uncompress some brotli file (generated by brotli cli) using the java client...

now I'm using zip as alternative.

@eustas
Copy link
Collaborator

eustas commented Nov 20, 2024

Got it. I'll prepare helper for reflection-less load-from-resources soon; the biggest question is - what build system (or binary repo) you use?

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

No branches or pull requests

2 participants