1744975 - Implement Tuple.prototype.* methods

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Nicolò Ribaudo]

The patches for 1730843 implement a complete version of the new Record/Tuple/Box primitives, but it doesn't include all the Tuple.prototype.* methods. I'm tracking their implementation separately because they are another self-contained part of the proposal.

This bug should be assigned to [e-mail] (I don't think I can assign it; I see the field grayed out)

Comment 1

[Tim Chevalier]

Attachment: Bug 1744975 - add ArrayLikeLength intrinsic r=mgaudet,jandem

Add an ArrayLikeLength intrinsic and call it from self-hosted Array methods.
For any Object o (that's not an ExtendedPrimitive), it just returns o's
length property. In the future, this will be modified to access an
ExtendedPrimitive's length from the internal slot rather than the property.
This is to allow reusing array code for Tuples while preserving the semantics
from the spec that built-in methods always use the internal length slot
rather than calling an overridden method.

Comment 2

[Tim Chevalier]

Attachment: Bug 1744975 - 1. Add error messages for tuple methods r=jandem

Depends on D134821

Comment 3

[Tim Chevalier]

Attachment: Bug 1744975 - 2. Throw type error when attempting to convert tuples to numbers r=jandem

As per spec 3.1.2

Depends on D134822

Comment 4

[Tim Chevalier]

Attachment: Bug 1744975 - 3. Add thisTupleValue() method to TupleObject r=jandem

Depends on D134823

Comment 5

[Tim Chevalier]

Attachment: Bug 1744975 - 4. Factor out ArrayReverseDenseKernel() r=jandem

This is useful for implementing the tuple reversed() method.

Depends on D134824

Comment 6

[Tim Chevalier]

Attachment: Bug 1744975 - 4. Handle Records and Tuples properly in js::ToStringSlow() r=jandem

Depends on D134825

Comment 7

[Tim Chevalier]

Attachment: Bug 1744975 - 5. Stringify Record/Tuple wrappers correctly in js::ObjectToSource() r=jandem

Depends on D134826

Comment 8

[Tim Chevalier]

Attachment: Bug 1744975 - 6. Add IsTuple and ThisTupleValue self-hosting intrinsics r=jandem

Depends on D134827

Comment 9

[Tim Chevalier]

Attachment: Bug 1744975 - 8. Add support for tuple iteration r=mgaudet,jandem

Depends on D134828

Comment 10

[Tim Chevalier]

Attachment: Bug 1744975 - 8. Allow array_join to be used for Tuples r=jandem

Depends on D134829

Comment 11

[Tim Chevalier]

Attachment: Bug 1744975 - 9. Expose array_toLocaleString r=jandem

This is in order to implement the toLocaleString method for tuples.

Depends on D134830

Comment 12

[Tim Chevalier]

Attachment: Bug 1744975 - 7. Add a TupleLength intrinsic r=jandem

This will be called by self-hosting tuple methods.

Depends on D134831

Comment 13

[Tim Chevalier]

Attachment: Bug 1744975 - 11. Handle tuple length properly in Array code r=jandem

Tuple self-hosted methods will rely on Array methods. These methods
should get the internal length slot when called on a Tuple, rather than
the length property.

Depends on D134832

Comment 14

[Tim Chevalier]

Attachment: Bug 1744975 - 8. Implement tuple prototype methods r=jandem

Depends on D134833

Comment 15

[Tim Chevalier]

Attachment: Bug 1744975 - 9. Implement Tuple.isTuple() static method r=jandem

Depends on D134834

Comment 16

[Tim Chevalier]

Attachment: Bug 1744975 - 10. Implement Tuple.from() static method r=jandem

Depends on D134835

Comment 17

[Tim Chevalier]

Attachment: Bug 1744975 - 15. Implement Tuple.of() static method r=jandem

Depends on D134836

Comment 18

[Tim Chevalier]

Attachment: Bug 1744975 - 12. Tests for section 6.1.2.1 of spec (initialization semantics) r=mgaudet,jandem

Depends on D134837

Comment 19

[Tim Chevalier]

Attachment: Bug 1744975 - 13. Conformance tests for Tuple constructor r=jandem

Depends on D134838

Comment 20

[Tim Chevalier]

Attachment: Bug 1744975 - 14. Add test for 9.1.1.1 of spec (Object.prototype.toString) r=jandem

Depends on D134839

Comment 21

[Tim Chevalier]

Attachment: Bug 1744975 - 15. Conformance tests for section 4.1.2.11 of proposal (out-of-range Tuple indices) r=jandem

Depends on D134840

Comment 22

[Tim Chevalier]

Attachment: Bug 1744975 - 16. Conformance tests for section 4.1.2.6 of proposal (setting Tuple properties) r=jandem

Depends on D134841

Comment 23

[Tim Chevalier]

Attachment: Bug 1744975 - Change type signatures of TupleObject::unbox(), TupleObject::maybeUnbox() r=jandem

• unbox() now returns a TupleType&
• maybeUnbox() now returns a Maybe<TupleType&>; in both cases, callers are responsible for rooting
• IsTuple() now takes a Value& rather than a HandleValue
• ThisTupleValue() now takes a Value& and returns a TupleType&

These changes are meant to minimize assumptions made by TupleObject and TupleType methods about their inputs, and shift responsibility for rooting onto the callers.

Comment 24

[Jan de Mooij [:jandem]]

I think I'd prefer implementing these builtins in self-hosted code, at least for now, without changing the existing Array builtins. This is all very perf-sensitive code so touching it as little as possible would be less risk for us.

I also think we should rely less on records/tuples being implemented as objects, as that should be just an implementation detail. For instance if we ever decide to implement records/tuples without using JSObject (similar to how JSString and BigInt don't use JSObject), ideally we wouldn't have to change much. I've been proposing we branch on the Value type more than on the object's class, but there are still some places I'd like to convert to that scheme.

Comment 25

[Tim Chevalier]

(In reply to Jan de Mooij [:jandem] from comment #24)

I think I'd prefer implementing these builtins in self-hosted code, at least for now, without changing the existing Array builtins. This is all very perf-sensitive code so touching it as little as possible would be less risk for us.

Which changes to the existing Array builtins are you referring to? The only one I would expect to have any potential impact is the initial patch (D134821) that changes O.length to ArrayLikeLength(O). That change can be discussed further, but first I wanted to make sure there wasn't anything else you had in mind.

I also think we should rely less on records/tuples being implemented as objects, as that should be just an implementation detail. For instance if we ever decide to implement records/tuples without using JSObject (similar to how JSString and BigInt don't use JSObject), ideally we wouldn't have to change much. I've been proposing we branch on the Value type more than on the object's class, but there are still some places I'd like to convert to that scheme.

I would also be curious about which particular changes relying on the implementation as objects you had in mind; while the decision to implement R/T as objects was made before I joined the project, one of the advantages of it that I can see is code re-use.

Comment 26

[Jan de Mooij [:jandem]]

(In reply to Tim Chevalier from comment #25)

Which changes to the existing Array builtins are you referring to? The only one I would expect to have any potential impact is the initial patch (D134821) that changes O.length to ArrayLikeLength(O). That change can be discussed further, but first I wanted to make sure there wasn't anything else you had in mind.

It's not just performance, it seems wrong to me that we have to make changes (that don't correspond to changes in the spec) to the array builtins because they're reused by the Tuple builtins. It'd be better to follow what we did for the similar typed array functions in js/src/builtin/TypedArray.js

What's really nice about the Tuple functions compared to the ancient Array builtins is that they're non-generic, so we could probably implement them much more efficiently than the generic implementation (although performance isn't a serious concern at this point).

Comment 27

[Tim Chevalier]

I've removed the patch in question because I've since learned that the spec is likely going to change to add length as an own property to Tuples. Thus, the array self-hosted built-ins don't need to change (with the exception of IsConcatSpreadable().) Patches are now ready for review.

Comment 28

[Jan de Mooij [:jandem]]

I think comment 26 still applies?

Comment 29

[Tim Chevalier]

(In reply to Jan de Mooij [:jandem] from comment #28)

I think comment 26 still applies?

Which changes do you have in mind?

Comment 30

[Jan de Mooij [:jandem]]

(In reply to Tim Chevalier from comment #29)

(In reply to Jan de Mooij [:jandem] from comment #28)

I think comment 26 still applies?

Which changes do you have in mind?

Let's avoid touching the array builtins. I'm also pretty sure we can implement these tuple methods more efficiently because they're non-generic so can be specialized for tuples.

Comment 31

[Tim Chevalier]

Attachment: Bug 1744975 - 11. Implement Tuple.of() static method r=jandem

Depends on D134836

Depends on D134836

Comment 32

[Tim Chevalier]

(In reply to Jan de Mooij [:jandem] from comment #30)

(In reply to Tim Chevalier from comment #29)

(In reply to Jan de Mooij [:jandem] from comment #28)

I think comment 26 still applies?

Which changes do you have in mind?

Let's avoid touching the array builtins. I'm also pretty sure we can implement these tuple methods more efficiently because they're non-generic so can be specialized for tuples.

OK, I've removed changes to the array built-ins, except for https://phabricator.services.mozilla.com/D134834#change-jdmntqC3ykIW , which is necessary in order to make Tuples treated as spreadable. Please take a look; thanks!

Comment 33

[Jan de Mooij [:jandem]]

Thanks! Sorry for the delay, I got side-tracked a bit but I'll try to get to this today or tomorrow.

Comment 34

[Tim Chevalier]

(In reply to Jan de Mooij [:jandem] from comment #33)

Thanks! Sorry for the delay, I got side-tracked a bit but I'll try to get to this today or tomorrow.

If I'm not mistaken, the whole stack is ready to land now. Let me know if anything else needs to be fixed before landing.

Comment 35

[Jan de Mooij [:jandem]]

(In reply to Tim Chevalier from comment #34)

(In reply to Jan de Mooij [:jandem] from comment #33)

Thanks! Sorry for the delay, I got side-tracked a bit but I'll try to get to this today or tomorrow.

If I'm not mistaken, the whole stack is ready to land now. Let me know if anything else needs to be fixed before landing.

Do you have a green try push? The latest one I saw had some failures AFAICT.

Comment 36

[Tim Chevalier]

Oops, I forgot to add the line to some of the tests that disables them if records and tuples are disabled. I'm running another try build now ( https://treeherder.mozilla.org/jobs?repo=try&revision=6624b5c895a119b7d51a644aed22d3e0a01d4d ).

Comment 37

[Tim Chevalier]

Green try push: https://treeherder.mozilla.org/push-health/push?repo=try&revision=6624b5c895a119b7d51a644aed22d3e0a01d4d19&testGroup=pr

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/146d317b2766
Change type signatures of TupleObject::unbox(), TupleObject::maybeUnbox() r=jandem
https://hg.mozilla.org/integration/autoland/rev/9bc676b9dab0
1. Add error messages for tuple methods r=jandem
https://hg.mozilla.org/integration/autoland/rev/ed07b1a7007f
2. Throw type error when attempting to convert tuples to numbers r=jandem
https://hg.mozilla.org/integration/autoland/rev/f3449321c6d4
3. Add thisTupleValue() method to TupleObject r=jandem
https://hg.mozilla.org/integration/autoland/rev/4e09070d2ee8
4. Handle Records and Tuples properly in js::ToStringSlow() r=jandem
https://hg.mozilla.org/integration/autoland/rev/d1e492caeb45
5. Stringify Record/Tuple wrappers correctly in js::ObjectToSource() r=jandem
https://hg.mozilla.org/integration/autoland/rev/9750b41619e9
6. Add IsTuple and ThisTupleValue self-hosting intrinsics r=jandem
https://hg.mozilla.org/integration/autoland/rev/1fdbcea41dfa
7. Add a TupleLength intrinsic r=jandem
https://hg.mozilla.org/integration/autoland/rev/325388373bcc
8. Implement tuple prototype methods r=jandem
https://hg.mozilla.org/integration/autoland/rev/de050c420c73
9. Implement Tuple.isTuple() static method r=jandem
https://hg.mozilla.org/integration/autoland/rev/0eb6cd59f507
10. Implement Tuple.from() static method r=jandem
https://hg.mozilla.org/integration/autoland/rev/2ad924ee5e6b
11. Implement Tuple.of() static method r=jandem
https://hg.mozilla.org/integration/autoland/rev/805a82e92070
12. Tests for section 6.1.2.1 of spec (initialization semantics) r=jandem
https://hg.mozilla.org/integration/autoland/rev/37ce48df24d8
13. Conformance tests for Tuple constructor r=jandem
https://hg.mozilla.org/integration/autoland/rev/4323ecb998a7
14. Add test for 9.1.1.1 of spec (Object.prototype.toString) r=jandem
https://hg.mozilla.org/integration/autoland/rev/28d75f32d2ce
15. Conformance tests for section 4.1.2.11 of proposal (out-of-range Tuple indices) r=jandem
https://hg.mozilla.org/integration/autoland/rev/395f90b6e880
16. Conformance tests for section 4.1.2.6 of proposal (setting Tuple properties) r=jandem

Comment 39

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/146d317b2766
https://hg.mozilla.org/mozilla-central/rev/9bc676b9dab0
https://hg.mozilla.org/mozilla-central/rev/ed07b1a7007f
https://hg.mozilla.org/mozilla-central/rev/f3449321c6d4
https://hg.mozilla.org/mozilla-central/rev/4e09070d2ee8
https://hg.mozilla.org/mozilla-central/rev/d1e492caeb45
https://hg.mozilla.org/mozilla-central/rev/9750b41619e9
https://hg.mozilla.org/mozilla-central/rev/1fdbcea41dfa
https://hg.mozilla.org/mozilla-central/rev/325388373bcc
https://hg.mozilla.org/mozilla-central/rev/de050c420c73
https://hg.mozilla.org/mozilla-central/rev/0eb6cd59f507
https://hg.mozilla.org/mozilla-central/rev/2ad924ee5e6b
https://hg.mozilla.org/mozilla-central/rev/805a82e92070
https://hg.mozilla.org/mozilla-central/rev/37ce48df24d8
https://hg.mozilla.org/mozilla-central/rev/4323ecb998a7
https://hg.mozilla.org/mozilla-central/rev/28d75f32d2ce
https://hg.mozilla.org/mozilla-central/rev/395f90b6e880