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

Proposal of new Object design #1354

Merged
merged 6 commits into from
Aug 27, 2021
Merged

Conversation

jedel1043
Copy link
Member

@jedel1043 jedel1043 commented Jun 22, 2021

This PR closes #591.

Currently some tests fail because we aren't considering the many overrides of Object internal functions that ExoticObjects do.

The discussion of this issue is in #591, and the proposed solution is to implement every override that the spec requires directly inside Object, considering every exotic and special object implementation. This works, but it's a little detrimental for the modularity of the design.

This PR is an alternative design based on dynamic function pointers that solves the same problem.
The rationale on why I used function pointers is in this line:

} else if proto.borrow().data.internal_methods.__get_prototype_of__ as usize
!= ordinary_get_prototype_of as usize

the spec states: "If p.[[GetPrototypeOf]] is not the ordinary object internal method defined in 10.1.1, set done to true.", and I couldn't find a way to express this check without using function pointers.

Please let me know what you think can be improved.

@jedel1043 jedel1043 force-pushed the object_proto branch 2 times, most recently from 37203b7 to ccde6ff Compare July 2, 2021 01:52
@Razican Razican requested a review from HalidOdat July 2, 2021 15:49
@jedel1043 jedel1043 force-pushed the object_proto branch 2 times, most recently from c465122 to a336961 Compare July 12, 2021 23:15
@Razican
Copy link
Member

Razican commented Jul 13, 2021

Test262 conformance changes:

Test result master count PR count difference
Total 78,897 78,897 0
Passed 27,936 27,936 0
Ignored 15,616 15,616 0
Failed 35,345 35,345 0
Panics 0 0 0
Conformance 35.41% 35.41% 0.00%

boa/src/object/object_.rs Outdated Show resolved Hide resolved
@jedel1043 jedel1043 force-pushed the object_proto branch 2 times, most recently from 291a960 to dcb6ad0 Compare July 28, 2021 23:31
@jedel1043 jedel1043 closed this Aug 17, 2021
@jedel1043 jedel1043 reopened this Aug 17, 2021
@jedel1043 jedel1043 force-pushed the object_proto branch 9 times, most recently from 4671305 to 4114e39 Compare August 17, 2021 20:35
@jedel1043
Copy link
Member Author

Not gonna lie, I expected a lot worse benckmarks with this new design, but there are some small gains and moderate losses. Maybe I overlooked some optimization so I'm still going to try to lower the perf diff.
@HalidOdat could I hear your opinion on this design? Should we sacrifice performance for api ergonomics?

@jedel1043 jedel1043 force-pushed the object_proto branch 6 times, most recently from 886febe to cb9d93d Compare August 18, 2021 06:58
@jedel1043
Copy link
Member Author

Ok, I made some optimizations and now the benchmarks have very few moderate regressions and some small to moderate improvements, so maybe we don't need to sacrifice performance with this design.

@jedel1043
Copy link
Member Author

@HalidOdat Rebased 😁

@jedel1043
Copy link
Member Author

jedel1043 commented Aug 24, 2021

Test262 conformance changes:

Test result master count PR count difference
Total 80,685 80,685 0
Passed 31,809 31,821 +12
Ignored 15,818 15,818 0
Failed 33,058 33,046 -12
Panics 2 2 0
Conformance 39.42% 39.44% +0.01%
Fixed tests (12):
test/built-ins/Object/keys/15.2.3.14-5-11.js [strict mode] (previously Failed)
test/built-ins/Object/keys/15.2.3.14-5-11.js (previously Failed)
test/built-ins/Object/create/properties-arg-to-object-non-empty-string.js [strict mode] (previously Failed)
test/built-ins/Object/create/properties-arg-to-object-non-empty-string.js (previously Failed)
test/built-ins/Object/assign/Source-String.js [strict mode] (previously Failed)
test/built-ins/Object/assign/Source-String.js (previously Failed)
test/built-ins/Object/entries/primitive-strings.js [strict mode] (previously Failed)
test/built-ins/Object/entries/primitive-strings.js (previously Failed)
test/built-ins/Object/values/primitive-strings.js [strict mode] (previously Failed)
test/built-ins/Object/values/primitive-strings.js (previously Failed)
test/built-ins/String/numeric-properties.js [strict mode] (previously Failed)
test/built-ins/String/numeric-properties.js (previously Failed)

@jedel1043
Copy link
Member Author

Ok, this should be good now

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

Wow impressive work. I'll continue the review later, as this is a huge diff!

boa/src/value/mod.rs Show resolved Hide resolved
boa/src/value/mod.rs Show resolved Hide resolved
boa/src/object/mod.rs Show resolved Hide resolved
boa/src/object/internal_methods/string.rs Outdated Show resolved Hide resolved
boa/src/builtins/function/mod.rs Show resolved Hide resolved
boa/src/builtins/object/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/string/mod.rs Show resolved Hide resolved
boa/src/builtins/string/mod.rs Show resolved Hide resolved
boa/src/object/internal_methods/mod.rs Show resolved Hide resolved
boa/src/object/internal_methods/mod.rs Show resolved Hide resolved
boa/src/object/internal_methods/array.rs Show resolved Hide resolved
@jedel1043 jedel1043 force-pushed the object_proto branch 4 times, most recently from ae5984f to ec43006 Compare August 26, 2021 01:50
@jedel1043
Copy link
Member Author

@Razican Ok, did a big documentation cleanup. Now every internal method and exotic internal method is documented. Also did a cleanup of some todos. Let me know it there's something else missing 😃

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

Looking good! I noticed there is this file that should probably not be committed: boa/my_trace.mm_profdata. Maybe we could add the extension to the gitignore?

I was also missing a link to the spec, but for the rest, it's ready to be merged :)

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

Looking good! I noticed there is this file that should probably not be committed: boa/my_trace.mm_profdata. Maybe we could add the extension to the gitignore?

Oops, let me add it to the .gitignore.

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

Everything looks great now!

@Razican Razican merged commit 8afd50f into boa-dev:master Aug 27, 2021
@jedel1043 jedel1043 deleted the object_proto branch August 27, 2021 12:52
@Razican Razican mentioned this pull request Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
execution Issues or PRs related to code execution technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Internal Object Methods
3 participants