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

Change type of prototypes of objects to Option<JsObject> #1640

Merged
merged 1 commit into from
Oct 6, 2021

Conversation

jedel1043
Copy link
Member

The objective of this PR is to try to make use of the null pointer optimization of the Option type, theoretically optimizing the access of the prototype of an object.

It changes the following:

  • Changes the type of prototype of objects from JsValue to JsObject
  • Creates a new type alias ObjectPrototype to avoid having to read Option<Option<JsObject>> in some places. (name subject to change, maybe? I was thinking of using plain Prototype but there's already a PROTOTYPE constant, and ObjectPrototype doesn't sound as good because it could be confused with the intrinsic prototypes of constructor objects. I'd like to hear your suggestions on this)

@jedel1043 jedel1043 added performance Performance related changes and issues technical debt labels Oct 6, 2021
@jedel1043 jedel1043 added this to the v0.14.0 milestone Oct 6, 2021
@jedel1043 jedel1043 marked this pull request as draft October 6, 2021 09:31
@github-actions
Copy link

github-actions bot commented Oct 6, 2021

Test262 conformance changes:

Test result main count PR count difference
Total 86,438 86,438 0
Passed 37,324 37,324 0
Ignored 19,022 19,022 0
Failed 30,092 30,092 0
Panics 0 0 0
Conformance 43.18% 43.18% 0.00%

@jedel1043 jedel1043 force-pushed the option-proto branch 2 times, most recently from e9ff234 to c3610a9 Compare October 6, 2021 11:00
@jedel1043 jedel1043 marked this pull request as ready for review October 6, 2021 11:05
@jedel1043 jedel1043 requested review from HalidOdat, Razican, RageKnify and raskad and removed request for HalidOdat October 6, 2021 11:05
Copy link
Member

@RageKnify RageKnify left a comment

Choose a reason for hiding this comment

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

It's always nice to use the type-system to ensure invariants, should result in faster and more memory efficient code.

@raskad
Copy link
Member

raskad commented Oct 6, 2021

Concerning the name: I think we should prefix it with Js. So JsObjectPrototype or just JsPrototype?

Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

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

Looks good to me. Really nice idea.

boa/src/object/mod.rs Outdated Show resolved Hide resolved
@jedel1043
Copy link
Member Author

Concerning the name: I think we should prefix it with Js. So JsObjectPrototype or just JsPrototype?

I like JsPrototype, it's clear and it doesn't overlap with the PROTOTYPE constant :)

@jedel1043 jedel1043 merged commit 510623b into main Oct 6, 2021
@jedel1043 jedel1043 deleted the option-proto branch October 6, 2021 18:55
@RageKnify RageKnify added the Internal Category for changelog label Jan 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Category for changelog performance Performance related changes and issues technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants