Closed Bug 1398928 Opened 2 years ago Closed 2 years ago

Fix the argument to SpeciesConstructor

Categories

(Core :: JavaScript: Standard Library, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: anba, Assigned: anba)

References

Details

(Whiteboard: [qf:p3])

Attachments

(1 file, 2 obsolete files)

because I didn't spot in while reviewing bug 1386534.

And make two easy performance improvements for Promise.all/race.
Attachment #8906787 - Flags: review?(till)
We may not be able to skip the creation of the resolving functions, but we can still skip creating the GetCapabilitiesExecutor function for the common case when the constructor argument is the built-in Promise constructor in Promise.race/all.

I've changed the arguments of CreateResolvingFunctions from Value types to Object types, so it's more easy to directly call this function from the new CreatePromiseWithDefaultResolutionFunctions method. 

And I've also changed the GetProperty(...) calls to no longer go through the Interpreter's GetProperty(...) function, because that allows us to skip this code [1].

[1] http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/js/src/vm/Interpreter.cpp#4394-4419
Attachment #8906793 - Flags: review?(till)
Priority: -- → P3
Whiteboard: [qf:p3]
Comment on attachment 8906787 [details] [diff] [review]
bug1398928-part1-species-constructor-arg.patch

Review of attachment 8906787 [details] [diff] [review]:
-----------------------------------------------------------------

Huh, yes. Thank you!
Attachment #8906787 - Flags: review?(till) → review+
Comment on attachment 8906793 [details] [diff] [review]
bug1398928-part2-perf-changes.patch

Review of attachment 8906793 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice! This is indeed an easy perf win that I hadn't thought of.

Given that we're solidly in the stabilization phase for 57, I think it'd be good to split this into a new bug and land it after the 57 branch, but still land part 1 here in time for 57.
Attachment #8906793 - Flags: review?(till) → review+
Blocks: 1401508
Attached patch bug1398928.patchSplinter Review
Drop "Part 1" from commit message, part 2 will be handled in bug 1401508. No change in behaviour, therefore carrying r+ from Till.
Attachment #8906787 - Attachment is obsolete: true
Attachment #8910229 - Flags: review+
Comment on attachment 8906793 [details] [diff] [review]
bug1398928-part2-perf-changes.patch

Part 2 gets moved to bug 1401508.
Attachment #8906793 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7aa9a38d74ba
Pass correct argument to SpeciesConstructor call. r=till
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7aa9a38d74ba
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.