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

Convert NativeObject from IdScriptableObject to lambda constructor. #1824

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

Conversation

aardvark179
Copy link
Contributor

@gbrail I'm creating this as a draft PR for now. There are 24 test failures from doc tests due to small changes in the toString results for various functions on object, and I'm not sure if we should work to reduce those differences or if we're fine with changing the tests.

I'll update this PR with benchmark comparisons when I've had time to run the full suite.

@gbrail
Copy link
Collaborator

gbrail commented Feb 11, 2025

This is directionally the right thing to do, and the right way to do it. As you can see from the test results, there are a bazillion legacy behaviors that need to be fixed when making a change like this, unfortunately. FWIW I was going to tackle fundamental classes like this later, but since you already did most of the work it'd be great to figure out what's holding back those tests that are now failing.

Also, after that's all done, please run:

./gradlew :tests:test --tests Test262SuiteTest -DupdateTest262properties

to regenerate the properties file!

@aardvark179
Copy link
Contributor Author

I’ve got benchmarks. Some better, some worse, but I think I understand why. Probably won’t get the test fixes done till the weekend.

@aardvark179 aardvark179 force-pushed the aardvark179-native-object-refactor branch from ccceda6 to 0d61dc9 Compare February 15, 2025 19:14
@aardvark179
Copy link
Contributor Author

aardvark179 commented Feb 16, 2025

@gbrail Everything is passing nicely except for one proxy test which looks at the result of toString on Proxy. I fixed the doc tests by making our lambdas functions look more like the old id functions when converted to strings, but that broke the NativeProxy test.

That would be simple to fix by just changing the test, but the better long term solution might be to make our function toString output match the 262 spec. I just didn't fancy change all the occurrences in the docs.

@rbri
Copy link
Collaborator

rbri commented Feb 16, 2025

That would be simple to fix by just changing the test, but the better long term solution might be to make our function toString output match the 262 spec.

i guess #1300 is exactly that story. maybe we have to address this in a separate PR

@aardvark179 aardvark179 force-pushed the aardvark179-native-object-refactor branch from dd79a9f to b5a00e2 Compare February 21, 2025 10:56
@aardvark179
Copy link
Contributor Author

aardvark179 commented Feb 21, 2025

@gbrail, @rbri Right, done the function toString changes in #1840, and rebased this PR on top of that.

@aardvark179 aardvark179 force-pushed the aardvark179-native-object-refactor branch from b5a00e2 to 32cf21c Compare February 24, 2025 22:17
@aardvark179 aardvark179 marked this pull request as ready for review February 25, 2025 18:16
@aardvark179
Copy link
Contributor Author

Okay, Benchmarks are broadly very similar (as you would expect) with the exception of the sun spider Nsieve benchmark which was showing a 30% regression, and some smaller regressions in other array heavy benchmarks. This was happening because IdScriptableObject does not override has(int, Scriptable), so when you do an_array[I] = thing the prototype check used to be against an empty slot map (all the prototype properties were ids) but is now against an embedded slot map so is more expensive.

I've added an optimisation to EmbeddedSlotMap to short circuit the query for integer indexes if none have been added to the map, and this recovers pretty much all the performance.

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