Closed Bug 1041586 Opened 6 years ago Closed 4 years ago

Implement Symbol.isConcatSpreadable

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: bzbarsky, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(6 files, 1 obsolete file)

We should have it for ArrayClass bits.
Blocks: 869376
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
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
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: dom-requests
This should be relatively simple with the new selfhosted concat.
Depends on: 1233642
Assignee: nobody → evilpies
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)
No longer blocks: es6
Attached 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.
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+
Keywords: leave-open
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+
Attachment #8739666 - Flags: review?(arai.unmht)
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+
Attached 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)
I think that test might be buggy sometimes? Try looked okay https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c3b1879918a actually.
I am just going to assume this test is broken, inserting a print makes the failure go away.
Flags: needinfo?(evilpies)
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.