Implement Symbol.isConcatSpreadable

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: bzbarsky, Assigned: evilpie)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla48
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(6 attachments, 1 obsolete attachment)

We should have it for ArrayClass bits.
Blocks: 869376
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]

Comment 1

4 years ago
Isn't it a dupe of Bug 1054758 ?
Yes, except filed a month earlier.  So the dup is the other way around.  ;)

Not that I care much as long as the dependencies are propagated to whichever bug is used here.
Duplicate of this bug: 1054758
Blocks: 1119779, 694100
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 4

4 years ago
Changing the implementation of concat for this is really simple. However concat is also inlined in Ion, and just checking for the array class isn't enough anymore.
As pointed out on public-script-coord, we should make @@isConcatSpreadable return undefined on cross-origin windows when we implement this.
Summary: Implement @@isConcatSpreadable or equivalent → Implement Symbol.isConcatSpreadable
Blocks: 1218972
(Assignee)

Comment 6

3 years ago
This should be relatively simple with the new selfhosted concat.
Depends on: 1233642
(Assignee)

Updated

3 years ago
Assignee: nobody → evilpies
(Assignee)

Comment 7

3 years ago
I was a bit upset that we actually hard-coded @@iterator as the first symbol, so I decided to automatically generate all the symbol atoms. So now you can add new symbols at the frond and end of the enum!
Attachment #8739665 - Flags: review?(jorendorff)
(Assignee)

Updated

3 years ago
No longer blocks: 694100
(Assignee)

Comment 9

3 years ago
Posted patch IsConcatSpreadable (obsolete) — Splinter Review
Normally we would be done now, sadly this causes a regression between 10-25% on a micro benchmark using concat. It seems like the O[@@isConcatSpreadable] lookup is not optimized away and even uses a property cache. I will have to look into completely eliminating that.
(Assignee)

Comment 10

3 years ago
Attachment #8739667 - Attachment is obsolete: true
Comment on attachment 8739665 [details] [diff] [review]
Autogenerate symbol names

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

Yes, of course. Thanks.
Attachment #8739665 - Flags: review?(jorendorff) → review+
(Assignee)

Updated

3 years ago
Keywords: leave-open
(Assignee)

Comment 13

3 years ago
This patch allows us to completely fold away O[Symbol.isContactSpreadable], this wins back all the performance on a micro benchmark. I tried running and didn't see any obvious, but the results are extremely noisy for me.
Attachment #8741039 - Flags: review?(jdemooij)
Comment on attachment 8741039 [details] [diff] [review]
Fold away property accesses to not-defined properties

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

Thanks, that's a nice optimization to have for other code as well.
Attachment #8741039 - Flags: review?(jdemooij) → review+
(Assignee)

Updated

3 years ago
Attachment #8739666 - Flags: review?(arai.unmht)
(Assignee)

Updated

3 years ago
Attachment #8739669 - Flags: review?(arai.unmht)
Attachment #8739666 - Flags: review?(arai.unmht) → review+
Comment on attachment 8739669 [details] [diff] [review]
IsConcatSpreadable

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

Looks good, assuming the testcase is in another patch :)
Attachment #8739669 - Flags: review?(arai.unmht) → review+
(Assignee)

Comment 19

3 years ago
Posted patch TestsSplinter Review
Attachment #8741906 - Flags: review?(arai.unmht)
Comment on attachment 8741906 [details] [diff] [review]
Tests

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

Nice testcases :D
Attachment #8741906 - Flags: review?(arai.unmht) → review+
sorry had to back this out for assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=25962392&repo=mozilla-inbound
Flags: needinfo?(evilpies)
(Assignee)

Comment 23

3 years ago
I think that test might be buggy sometimes? Try looked okay https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c3b1879918a actually.
(Assignee)

Comment 25

3 years ago
I am just going to assume this test is broken, inserting a print makes the failure go away.
Flags: needinfo?(evilpies)
(Assignee)

Comment 26

3 years ago
It seems like this test case has been causing problems for a long time. I have to admit, I am not interested in fixing this just to get this bug finished.
Attachment #8742864 - Flags: review?(jdemooij)
Comment on attachment 8742864 [details] [diff] [review]
Prevent jit compilation for one expression-autopsy test

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

I'm still a bit curious why your patch exposes this, but given that we have seen this failure before and investigated it last time, this makes sense.
Attachment #8742864 - Flags: review?(jdemooij) → review+

Comment 29

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bb2748e05cab
https://hg.mozilla.org/mozilla-central/rev/1ddd1d9af872
https://hg.mozilla.org/mozilla-central/rev/219bb598b6ad
https://hg.mozilla.org/mozilla-central/rev/f569192eb5e8
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.