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

Performance: Lazy load Charset in EciMode #107

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

Conversation

uwolfer
Copy link
Contributor

@uwolfer uwolfer commented Mar 15, 2024

In my profilings, Symbol#eciProcess calling Charset#forName shows up quite slow (50ms). With this optimization, it no longer shows up, thus Pdf417#encode goes down from 190ms to 140ms.

Before:
Screenshot_20240315_233209

After:
Screenshot_20240315_233439

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 92.85714% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 87.26%. Comparing base (1a8bfa2) to head (6be3c80).

Files Patch % Lines
...rc/main/java/uk/org/okapibarcode/util/EciMode.java 92.15% 1 Missing and 3 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #107      +/-   ##
============================================
+ Coverage     87.25%   87.26%   +0.01%     
  Complexity     4299     4299              
============================================
  Files            68       68              
  Lines         13934    13938       +4     
  Branches       3254     3254              
============================================
+ Hits          12158    12163       +5     
+ Misses         1126     1125       -1     
  Partials        650      650              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gredler
Copy link
Collaborator

gredler commented Mar 18, 2024

I'm not sure this is worth the hassle, this initialization only happens once in the lifetime of the VM, right? Were you benchmarking a single call to createStructuredAppendSymbols?

@uwolfer
Copy link
Contributor Author

uwolfer commented Mar 19, 2024

I'm not sure this is worth the hassle, this initialization only happens once in the lifetime of the VM, right? Were you benchmarking a single call to createStructuredAppendSymbols?

Yes, I think this only is expensive once per VM lifetime.

Remark: In my tax use-case, the reference implementation is a Java CLI tool, which starts once per tax report PDF. That means, this initialization happens once per PDF. It is not too much of course, but in case you generate thousands of PDFs it still sums up a bit.

If you feel like it blows up the code too much, I am also OK with closing this PR.

@gredler
Copy link
Collaborator

gredler commented Mar 20, 2024

Understood -- let's keep this one open as a future option, but I'm not sure it's worth it right now. By the way, have you tried Graal for this use case? It might be a good fit...

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.

3 participants