Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Consstring optimization #1000

Merged
merged 4 commits into from
Jul 24, 2021
Merged

Consstring optimization #1000

merged 4 commits into from
Jul 24, 2021

Conversation

rbri
Copy link
Collaborator

@rbri rbri commented Jul 23, 2021

Two minor optimizations

  • there is no need to call a method because a simple cast is sufficient here
  • the second one is more complex, in the else case we already have a parameter conversion loop that also flattens the ConsStrings. Because of this we do not need to run the loop for this branch

Have done the usual test with my HtmlUnit test suite a found no notable performance change. But reducing i think we have to do this to reduce the load a bit.

@rbri
Copy link
Collaborator Author

rbri commented Jul 23, 2021

Sorry, looks like the second part introduces a problem. have to investigate this....

rbri added 2 commits July 23, 2021 10:50
My last commit introduces a problem in case the called method expects an object instead of a string. In this case the conversion from ConsString to String was missing. This was no problem before because of the separate conversion loop.
Have added a test case for this.

While investigating the issue if found another code path that leaks a ConsString into Java methods. This problem was NOT introduces by my optimization - it was there before. Have added a test for this also.

Luckily the fix fixes both problems and the conversion of values from JS to Java is now more consistent.
@rbri
Copy link
Collaborator Author

rbri commented Jul 23, 2021

Hope this is now complete - waiting for feedback.

Copy link
Collaborator

@gbrail gbrail left a comment

Choose a reason for hiding this comment

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

Both of these optimizations look correct to me. Thanks!

@rbri
Copy link
Collaborator Author

rbri commented Jul 23, 2021

Thanks will merge this also tomorrow (if no one complains)

@rbri rbri merged commit 76b37a1 into mozilla:master Jul 24, 2021
@rbri rbri deleted the consstring_optimization branch July 24, 2021 09:30
@p-bakker p-bakker added this to the Release 1.7.14 milestone Oct 13, 2021
@p-bakker p-bakker added the Performance Issues related to the performance of the Rhino engine label Oct 13, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Performance Issues related to the performance of the Rhino engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants