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

fix(boa): 15.4.4.19-8-c-ii-1.js #1351

Merged
merged 8 commits into from
Jul 24, 2021
Merged

fix(boa): 15.4.4.19-8-c-ii-1.js #1351

merged 8 commits into from
Jul 24, 2021

Conversation

neeldug
Copy link
Contributor

@neeldug neeldug commented Jun 20, 2021

This Pull Request fixes/closes #1306.

It changes the following:

  • skips undefined values on callback

todo: speedup execution of construct array, currently stalls for large arrays of mainly undefined values.

@neeldug
Copy link
Contributor Author

neeldug commented Jun 20, 2021

@Razican this will still stall on execution due to the utility function for creating array objects being super slow, not totally familiar with JS array objects and so not too sure if there's a way to avoid iterating through the entire array.

The specific call that's taking a huge amount of time, considering the required iterations are 1M can be found here.

@Razican
Copy link
Member

Razican commented Jun 21, 2021

Test262 conformance changes:

Test result master count PR count difference
Total 78,897 78,897 0
Passed 28,204 28,270 +66
Ignored 15,616 15,614 -2
Failed 35,077 35,013 -64
Panics 2 2 0
Conformance 35.75% 35.83% +0.08%
Fixed tests:
test/built-ins/Array/prototype/map/15.4.4.19-8-b-14.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-b-14.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-4.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-4.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-2.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-2.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-11.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-11.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-3.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-3.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-5.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-5.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-6.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-6.js (previously Failed)
test/built-ins/Array/prototype/map/create-species-non-ctor.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/create-species-non-ctor.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-c-ii-12.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-c-ii-12.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-14.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-14.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-c-ii-11.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-c-ii-11.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-b-10.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-b-10.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-4-15.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-4-15.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-b-8.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-b-8.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-10.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-10.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-c-ii-6.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-c-ii-6.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-1.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-1.js (previously Failed)
test/built-ins/Array/prototype/map/create-proto-from-ctor-realm-non-array.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/create-proto-from-ctor-realm-non-array.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-9.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-9.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-13.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-13.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-8.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-1-8.js (previously Failed)
test/built-ins/Array/prototype/map/create-ctor-non-object.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/create-ctor-non-object.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-4-1.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-4-1.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-c-ii-1.js [strict mode] (previously Ignored)
test/built-ins/Array/prototype/map/15.4.4.19-8-c-ii-1.js (previously Ignored)
test/built-ins/Array/prototype/map/create-species-poisoned.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/create-species-poisoned.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-b-3.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-b-3.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-b-1.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-b-1.js (previously Failed)
test/built-ins/Array/prototype/map/create-ctor-poisoned.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/create-ctor-poisoned.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-b-9.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-b-9.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-c-ii-19.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-c-ii-19.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-c-ii-13.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-c-ii-13.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-9.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-9.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-c-ii-23.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-c-ii-23.js (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-4.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/map/15.4.4.19-8-4.js (previously Failed)
Broken tests:
test/built-ins/Array/prototype/map/create-proto-from-ctor-realm-array.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/map/create-proto-from-ctor-realm-array.js (previously Passed)

@Razican
Copy link
Member

Razican commented Jun 21, 2021

@Razican this will still stall on execution due to the utility function for creating array objects being super slow, not totally familiar with JS array objects and so not too sure if there's a way to avoid iterating through the entire array.

The specific call that's taking a huge amount of time, considering the required iterations are 1M can be found here.

We can still ignore it until we figure this out. Give a look to the results, though, since many tests seem failing.

@neeldug

This comment has been minimized.

@neeldug
Copy link
Contributor Author

neeldug commented Jun 22, 2021

@RageKnify @Razican fixed it for all of map now, should be in line with spec, overall map conformance is 89.55%, which I believe is higher than before.

@Razican
Copy link
Member

Razican commented Jun 22, 2021

@RageKnify @Razican fixed it for all of map now, should be in line with spec, overall map conformance is 89.55%, which I believe is higher than before.

Great, thank you! I updated conformance numbers, and it seems there is only one extra broken test. Could you check why?

@neeldug
Copy link
Contributor Author

neeldug commented Jun 22, 2021

Great, thank you! I updated conformance numbers, and it seems there is only one extra broken test. Could you check why?

Pulled it into debugger, the text shows up as "Uncaught "TypeError": "Symbol.species must be a constructor"", which is actually expected, as before Boa didn't use the specified array_species_create function, which currently skips step 4, as seen here. I could implement this here, or on a separate PR since it might end up being more involved, not really too sure about how realms work in general. But either way, I think that this PR should be good to go for merging. @Razican

Looking more into this, it would require this to be implemented too, started trying to work on it at the moment, but it's starting to become more heavier than I think is within the bounds of this PR.

@neeldug
Copy link
Contributor Author

neeldug commented Jun 26, 2021

@Razican I think above specifies it, it's essentially due to Realms being unimplemented under array_species_create as I mentioned before. I believe this functionality wasn't present in the previous version but was rather a false positive, or a seeming pass. I think this PR is ready for merge.

Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

Look good to me! :)

Just needs a rebase

@HalidOdat HalidOdat added the builtins PRs and Issues related to builtins/intrinsics label Jul 21, 2021
@HalidOdat HalidOdat added this to the v0.13.0 milestone Jul 21, 2021
@neeldug neeldug force-pushed the fix-map branch 2 times, most recently from c5515a0 to 9ced066 Compare July 21, 2021 19:13
@neeldug neeldug requested review from HalidOdat and Razican July 21, 2021 19:21
Copy link
Member

@jasonwilliams jasonwilliams left a comment

Choose a reason for hiding this comment

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

just needs a rebase

@Razican Razican merged commit a4a76fe into boa-dev:master Jul 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some Test262 tests run forever
4 participants