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

JavaScriptEvaluator.getTestEvalResult blocks threads while concurrent validation #1408

Closed
SalomScala opened this issue Feb 13, 2024 · 2 comments
Assignees

Comments

@SalomScala
Copy link
Contributor

SalomScala commented Feb 13, 2024

In our scenario we parallize validations with 20 Threads in a single JVM.
We recognized a big bottleneck in the JavaScriptEvaluator.getTestEvalResult-Method, since it is synchronized and only one thread at a time can evaluate its result.

To show the problem I have attached

I have tried to fix it locally by adjusting JavaScriptEvaluator to use ThreadLocals.
I got the following results:

Testscenario:

  • Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
  • 20 parallel threads
  • 100 validations per thread

Result before optimization:

  • 537 seconds in total
  • 268 milliseconds per validation
  • 4 validations per second

Result after optimization:

  • 114 seconds in total after optimization
  • 57 milliseconds per validation after optimization
  • 17 validations per second

Thus an validation speed improvement of 325% in the concurrent case with 20 threads.

As far as I understand the JavaScriptEvaluator code, the change should be ok.
All testcases work and the rhino javascript engine can execute scripts in parallel.

But maybe you have further insight, why the synchronisation was used?
It got introduced in #960 because multithreading was not possible before.
Now I would like to improve the performance for multithreading.

I have created pull request #1409

I welcome any feedback.
Thanks.

@bdoubrov
Copy link
Contributor

Thanks a lot, @SalomScala ! Looks like a great optimization indeed! We have enabled thread-safety of veraPDF only recently and there are still places that need improvements. We'll review and merge your PR if all goes well.

@MaximPlusov
Copy link
Contributor

Included into release 1.26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants