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

Optimize extractSetMethod(...) and findFunction(...) #1804

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ZZZank
Copy link

@ZZZank ZZZank commented Jan 26, 2025

  • setter extracting is rewritten to do setter scanning in 1 pass instead of 2 passes
  • a helper method failFastConversionWeights(...) that can compute conversion weights in batch
  • computed weights from failFastConversionWeights(...) will now be cached to prevent computing weight again in preferSignature()

@ZZZank
Copy link
Author

ZZZank commented Jan 27, 2025

I've ran a test for these changes (https://github.com/ZZZank/rhino-mozilla/actions/runs/12994132974), seems to be working quite well

@gbrail
Copy link
Collaborator

gbrail commented Jan 28, 2025

This looks fine to me, although I don't consume this bit of Rhino functionality myself much anymore and it'd be nice to see if someone else could check it out.

Also, could you please run "./gradlew test testCodeCoverageReport" and take a look at the result (which is in tests/build/reports/jacoco/testCodeCoverageReport) for the parts that you changed?

I do see a big "we should never get here" in your code, and I won't worry as much about coverage there, but there are a few places where your new code checks for things and we never get there in the tests. It's not much but given how complex this code is I would feel better if you took at look at the coverage for the stuff you touched and see if you can cover as many of the edge cases as you can. Thanks!

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.

2 participants