Implement Symbol.isConcatSpreadable

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: bzbarsky, Assigned: evilpie)

Tracking

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

unspecified
mozilla48
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, es6
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
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: es6
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+
You need to log in before you can comment on or make changes to this bug.