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

lazy builtins #3973

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

lazy builtins #3973

wants to merge 9 commits into from

Conversation

jasonwilliams
Copy link
Member

@jasonwilliams jasonwilliams commented Aug 29, 2024

Lazy Builtins

So, this creates 2 new data structs which are used inside of JS Objects. LazyBuiltIn and LazyPrototype.
Instead of calling initialize on array for example, we will setup array with a constructor that has a LazyBuiltin object and a prototype which lazy a LazyPrototype. If any properties or the constructor itself is called then we will call init at that point.

For now I've started with:

  • Array

I also updated the VSCode task to always choose the debug/script.js path, as it makes debugging much faster

@jasonwilliams jasonwilliams mentioned this pull request Aug 29, 2024
core/engine/src/builtins/function/mod.rs Outdated Show resolved Hide resolved
core/engine/src/object/builtins/lazy_builtin.rs Outdated Show resolved Hide resolved
core/engine/src/object/builtins/lazy_builtin.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 9, 2024

Codecov Report

Attention: Patch coverage is 46.18182% with 148 lines in your changes missing coverage. Please review.

Project coverage is 52.89%. Comparing base (6ddc2b4) to head (7f07739).
Report is 286 commits behind head on main.

Files with missing lines Patch % Lines
core/engine/src/object/builtins/lazy_prototype.rs 0.00% 62 Missing ⚠️
core/engine/src/object/builtins/lazy_builtin.rs 39.02% 50 Missing ⚠️
...engine/src/object/builtins/lazy_array_prototype.rs 67.74% 20 Missing ⚠️
core/engine/src/realm.rs 55.00% 9 Missing ⚠️
core/engine/src/object/jsobject.rs 63.63% 4 Missing ⚠️
core/engine/src/builtins/function/mod.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3973      +/-   ##
==========================================
+ Coverage   47.24%   52.89%   +5.65%     
==========================================
  Files         476      486      +10     
  Lines       46892    47127     +235     
==========================================
+ Hits        22154    24928    +2774     
+ Misses      24738    22199    -2539     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jasonwilliams jasonwilliams added the performance Performance related changes and issues label Nov 9, 2024
@jasonwilliams jasonwilliams marked this pull request as ready for review November 9, 2024 22:06
@jedel1043
Copy link
Member

Test262 conformance changes

Test result main count PR count difference
Total 48,494 48,494 0
Passed 43,609 43,581 -28
Ignored 1,500 1,500 0
Failed 3,385 3,413 +28
Panics 0 2 +2
Conformance 89.93% 89.87% -0.06%
Broken tests (26):
test/language/statements/for-of/arguments-unmapped-mutation.js (previously Passed)
test/language/statements/for-of/arguments-unmapped-aliasing.js (previously Passed)
test/language/statements/for-of/arguments-mapped-aliasing.js (previously Passed)
test/language/statements/for-of/arguments-mapped-mutation.js (previously Passed)
test/language/statements/for-of/arguments-unmapped.js (previously Passed)
test/language/statements/for-of/arguments-mapped.js (previously Passed)
test/built-ins/ArrayIteratorPrototype/next/args-unmapped-truncation-before-exhaustion.js (previously Passed)
test/built-ins/ArrayIteratorPrototype/next/args-mapped-iteration.js (previously Passed)
test/built-ins/ArrayIteratorPrototype/next/args-unmapped-iteration.js (previously Passed)
test/built-ins/ArrayIteratorPrototype/next/args-unmapped-expansion-before-exhaustion.js (previously Passed)
test/built-ins/ArrayIteratorPrototype/next/args-mapped-expansion-after-exhaustion.js (previously Passed)
test/built-ins/ArrayIteratorPrototype/next/args-unmapped-expansion-after-exhaustion.js (previously Passed)
test/built-ins/ArrayIteratorPrototype/next/args-mapped-expansion-before-exhaustion.js (previously Passed)
test/built-ins/ArrayIteratorPrototype/next/args-mapped-truncation-before-exhaustion.js (previously Passed)
test/built-ins/String/prototype/S15.5.4_A2.js (previously Passed)
test/built-ins/String/prototype/S15.5.4_A3.js (previously Passed)
test/built-ins/String/prototype/S15.5.4_A1.js (previously Passed)
test/built-ins/Array/isArray/15.4.3.2-0-5.js (previously Passed)
test/built-ins/Array/prototype/exotic-array.js (previously Passed)
test/built-ins/Array/prototype/filter/create-proto-from-ctor-realm-array.js (previously Passed)
test/built-ins/Array/prototype/concat/create-proto-from-ctor-realm-array.js (previously Passed)
test/built-ins/Array/prototype/slice/create-proto-from-ctor-realm-array.js (previously Passed)
test/built-ins/Array/prototype/map/create-proto-from-ctor-realm-array.js (previously Passed)
test/built-ins/Array/prototype/splice/create-proto-from-ctor-realm-array.js (previously Passed)
test/built-ins/TypedArray/prototype/toString/BigInt/detached-buffer.js (previously Passed)
test/built-ins/TypedArrayConstructors/ctors/no-species.js (previously Passed)
New panics (2):
test/built-ins/Array/prototype/forEach/S15.4.4.18_A1.js (previously Passed)
test/built-ins/Array/prototype/forEach/S15.4.4.18_A2.js (previously Passed)

@jedel1043
Copy link
Member

Some tests regressed thanks to the changes. Is it potentially a typo/copypaste error, or do we need to go back to the drawing board?

@hansl
Copy link
Contributor

hansl commented Nov 10, 2024

I'll hold my review until it's up to date with main and green.

…eping for now.

Exotic objects need a specific Lazy equivalent so we can identify them in brand checks
)
}

pub(crate) fn native_function_construct_inner(
Copy link
Member Author

Choose a reason for hiding this comment

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

So there's been some (minor) performance regressions with this PR and I think its related to this, I'm going to assume because its not [inline]'d this is now calling 2 functions instead of 1, and because its on a hotpath its actually slowed it down. Ill try adding the inline attribute and see if thats what it is, but leaving this here as a reminder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance related changes and issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants