Closed
Bug 1265307
Opened 8 years ago
Closed 8 years ago
Delay selfhosting regexp and ES6 regexp conformity for a release
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: h4writer, Assigned: h4writer)
References
Details
Attachments
(5 files, 5 obsolete files)
3.86 KB,
text/plain
|
Details | |
3.77 KB,
patch
|
Details | Diff | Splinter Review | |
4.33 KB,
text/plain
|
Details | |
402.71 KB,
patch
|
h4writer
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
403.51 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
Bug 887016 implements ES6 RegExp.prototype methods and change String.prototype methods to delegate. This has landed in mozilla 48.
This bug has ups and downs:
- Selfhosting this piece of code!
- ES6 conformity, I think 2-3% improvement on http://kangax.github.io/compat-table/es6/
- Perf regressions on kraken (4%), ss (30%), octane (8.6%), jetstream (3%), browsermark (noisy)
- Stability issues.
On this day (merge day), the status is as following:
- All known stability issue got acknowledged and are fixed or close to fixing.
- Though fuzzers haven't been able to fuzz quite long (20 days or so)
- Given the spec change, regressions could be unavoidable.
- Perf regressions are mostly still on the draw-board. (bug 1264264 for ss, bug 1263549 for octane, bug 1263794, thinking about prepopulating selfhosted functions with TI info ...)
Discussion with efaust, jandem and jorendorff showed two potential paths forward.
1) Keep this bug in tree and let this go to aurora (dev edition). That way the dev edition has the ES6 improvements already. At the same time fix the stability and perf regressions and uplift to aurora. On merge to beta, decide again if we must backout.
2) Keep this bug in tree, but backout on aurora.
Even though we want 100% ES6 compatibility as soon as possible, most want to go for (2). This e.a. because of the quality initiative. Backing out on aurora gives us another few weeks to stabilize and bring performance closer to before this bug.
I'll create a patch to backout this on aurora and land this after the merge.
Assignee | ||
Comment 1•8 years ago
|
||
Evilpie just mentioned merge day has been delayed for a week. As a result this bug too ;)
Assignee | ||
Comment 2•8 years ago
|
||
Arai mentioned that there was a concern wrt not landing all @@species at once. Backing out this patch on merge, would mean we would loose the RegExp[@@species], but still having all other @@species implemented. (For just one release). Would that be an issue or can we do that? If we cannot do that, what are the alternatives?
Flags: needinfo?(till)
Flags: needinfo?(ljharb)
Comment 3•8 years ago
|
||
Given my past experience with @@species for typed arrays (bug 1145058), and thinking of a recent case of someone feature-testing for (I think?) something Symbol.* implying its support/existence everywhere, partial support seems very unwise to me. Based on the summary in comment 0 alone, it does seem to make sense to me to hold back on things for a release. And if that means RegExp[@@species] must be removed, I think that means all @@species support must too be removed.
This unfortunately seems like quite the mess. :-( Is there any chance at all of doing a "backout" that would have both code paths remain in the tree, with a single switch to flip to move between old/new implementations? Seems risky to have deal-with-the-whole-world issues here if we can avoid them.
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> Given my past experience with @@species for typed arrays (bug 1145058), and
> thinking of a recent case of someone feature-testing for (I think?)
> something Symbol.* implying its support/existence everywhere, partial
> support seems very unwise to me. Based on the summary in comment 0 alone,
> it does seem to make sense to me to hold back on things for a release. And
> if that means RegExp[@@species] must be removed, I think that means all
> @@species support must too be removed.
Even if it is only RegExp[@@species] ?
I would think the @@species is mostly used on other objects.
Do we have knowledge about cases that use the RegExp[@@species].
Note: I would prefer to only backout that part. Backing out the others is possible,
but will take more time and I want to be absolutely sure that backing out the
other stuff is really needed.
> This unfortunately seems like quite the mess. :-( Is there any chance at
> all of doing a "backout" that would have both code paths remain in the tree,
> with a single switch to flip to move between old/new implementations? Seems
> risky to have deal-with-the-whole-world issues here if we can avoid them.
It is a big change and it would be quite hard to have both implementation simultaneous.
I think the backout is easier, since it should definitely get merged next time around.
The big perf regressions should be gone and even if there is still a regression,
we will probably just take it. Maintaining both code paths for one slip seems a lot of work.
Comment 5•8 years ago
|
||
Note that, even though the new Promise implementation will probably miss the current train, the DOM Promise implementation supports @@species, too. I think at this point the ship has pretty much sailed for shipping this all at once. Also, there's a good argument to be made that it might be acceptable for library/polyfill authors - who're very likely to be the only ones having to really care about this - to carry some of the weight here by doing ever so slightly more complex feature detection (on the order of a few lines of very simple code, note). The upside is that we can ship new functionality as it's ready instead of blocking on things not being finished elsewhere.
Flags: needinfo?(till)
Sadly it's more complex than that. I can fix es6-shim, but thousands of sites currently host a version that assumes the presence of Symbol.species implies 100% support.
Shipping partial support will almost certainly break things. I'll research more fully today, but I do think it is a very big problem to ship any well-known Symbol without complete support.
Flags: needinfo?(ljharb)
Comment 7•8 years ago
|
||
> thousands of sites currently host a version that assumes the presence of Symbol.species implies 100% support
Symbol.species is present in the Firefox 45 release, which shipped over a month ago. In that release, I believe Promise is the only thing that actually supports @@species in any way.
So are we talking about unshipping Symbol.species and backporting that to 46 and later? Do we have any evidence of actual sites breaking in the current state of things? I'm totally willing to believe they exist, but did no one bother to report any of them? :(
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #7)
> So are we talking about unshipping Symbol.species and backporting that to 46
> and later? Do we have any evidence of actual sites breaking in the current
> state of things? I'm totally willing to believe they exist, but did no one
> bother to report any of them? :(
The question was if backing out RegExp[@@species] (in FF48) means we need to unship more?
The current knowledge was that not landing all @@species at the same time would break things.
Currently we (me) was thinking specifically about @@species of Array, TypedArray, String/RegExp.
Which all landed in FF48. (bug 1165052 and bug 1165053).
So the question was if we could only backout bug 887016 or also needed to backout bug 1165052 and bug 1165053.
> Symbol.species is present in the Firefox 45 release, which shipped over a
> month ago. In that release, I believe Promise is the only thing that
> actually supports @@species in any way.
So it seems we already have a precedent?
And we could go ahaed and only backout RegExp[@@species].
Is the only remaining reason to back it out a slight performance regression?
I've investigated, and I don't think es6-shim will break (at any point in its version history) by having @@species implemented on some builtins but not others (provided that any implementation on a builtin is complete). That said, correctness should be much more important than performance - why not let it ride, and attempt to improve the performance afterwards?
Comment 10•8 years ago
|
||
jharb, you're arguing that we should ship a feature that has stability issues in the interest of "correctness".
Please let us work here. We have to do what's best for users, and we understand that part of that is not breaking sites that use es6-shim. However, that can't be our sole concern.
Comment 11•8 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #8)
> The question was if backing out RegExp[@@species] (in FF48) means we need to
> unship more?
> [...]
> So it seems we already have a precedent?
> And we could go ahaed and only backout RegExp[@@species].
Let's try it. If things break, then we have to decide between pressing forward, backing out other @@species support, or just commenting out other @@species support.
Comment 12•8 years ago
|
||
I'm with Jason. In light of comment 7, I'd be shocked if the world ended. We are talking about backing something out 3 releases later. If we haven't heard the world ending in release in the last 5 weeks, we can believe that we can have incomplete support a little longer.
Comment 13•8 years ago
|
||
jorendorff: ah, i'm only implying that if the stability issues have been resolved, as some of the comments imply.
Certainly since it won't break users, and hasn't, I don't have any technical argument to not back it out. As for "let us work here", I was invited to comment, and I'm pretty sure that's what I'm trying to help you all do. I didn't notice the 30% regression in the initial comment (i thought it said 3) and that's clearly impactful. Thanks for the RFC!
Assignee | ||
Updated•8 years ago
|
Comment 14•8 years ago
|
||
SunSpider regression is now 16% on m-i.
Kraken is still 4%.
and I'm not sure where to read about Octane. I don't see 8.4% difference in the graph.
https://arewefastyet.com/#machine=28
Assignee | ||
Comment 15•8 years ago
|
||
This is the current situation:
At landing Now (23/04/2016)
browsermark-Javascript String Chat 2.1: 20% improvement 20% improvement
browsermark-Javascript String Filter 2.1: 32% regression 6% regression on ubuntu, mac, 11% impro on windows
browsermark-Javascript Array Weighted 2.1 42% regression 42% regression
browsermark-DOM Create Source 2.1: 33% regression 33% regression
kraken: 1.6% regression fixed!
Octane-regexp: 8% regression (64only) 12% improvement
Sunspider: 30% regression 16% regression (10% regression if bug 1266676 lands)
jetstream 3% regression 3% regression
Note: the kraken regression was apparently bug 887016, but mostly the array selfhosting stuff like Array.prototype.concat.
Note: octane-regexp regression was 64bit only
Octane and kraken are good. Sunspider will have a regression. I think that we can take the 10%. Hope to land bug 1266676 soonish again.
The remaining issues are browsermark and jetstream. I think we can get the jetstream fixed and uplifted. I fear for browsermark.
This gives us the current situation:
- Without backing out: browsermark and ss regression.
- With backing out: Regexp[species] delayed for one release.
That means we are getting close to not having to definitely not having to backout, but we are there not yet. Hmm.
For jetstream fixing, means the same as sunspider:
- crypto-aes
- date-format-tofte (which will already improve with bug 1266676)
- regex-dna
- tagcloud
Assignee | ||
Comment 16•8 years ago
|
||
For some reason the compare OS slaves haven't returned from the octane-regexp regression:
https://arewefastyet.com/#machine=31&view=single&suite=octane&subtest=RegExp
Quite strange since win 8 slave shows an improvement?
Needinfo myself to check this.
Flags: needinfo?(hv1989)
Assignee | ||
Comment 17•8 years ago
|
||
Opened bug 1267200 for the regexp regression on compare os
Depends on: 1267200
Flags: needinfo?(hv1989)
Assignee | ||
Comment 18•8 years ago
|
||
Assignee: nobody → hv1989
Attachment #8746072 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 19•8 years ago
|
||
Bug 1263340 is integral depending on 887016. That also needs to get backed out. Also some small changes were needed to get it compiling
Attachment #8746074 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 20•8 years ago
|
||
Bug 1263558 renamed FastInvokeGuard to FastCallGuard. Fixing that.
Attachment #8746075 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 21•8 years ago
|
||
Assignee | ||
Comment 22•8 years ago
|
||
Assignee | ||
Comment 23•8 years ago
|
||
Assignee | ||
Comment 24•8 years ago
|
||
Assignee | ||
Comment 25•8 years ago
|
||
As summary: this bug delays the following to FF49. I'll update the documentation upon landing:
RegExp.prototype[@@match]() (Firefox 48)
RegExp.prototype[@@replace]() (Firefox 48)
RegExp.prototype[@@search]() (Firefox 48)
RegExp.prototype[@@split]() (Firefox 48)
get RegExp[@@species] (Firefox 48)
Symbol.replace (Firefox 48)
Symbol.search (Firefox 48)
Symbol.split (Firefox 48)
Comment 26•8 years ago
|
||
please consider uplifting this to aurora (48) as we have regressions on there that will benefit from this.
Assignee | ||
Comment 27•8 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #26)
> please consider uplifting this to aurora (48) as we have regressions on
> there that will benefit from this.
It is the intention to only land this on aurora ;)
Comment 28•8 years ago
|
||
will there be a fix on trunk or other changes to address the perf regression this will fix on aurora?
Assignee | ||
Comment 29•8 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #28)
> will there be a fix on trunk or other changes to address the perf regression
> this will fix on aurora?
We will keep this the regressions (and currently also improvements) on trunk and are actively tackling the performance issues on trunk.
Comment 30•8 years ago
|
||
Comment on attachment 8746072 [details] [diff] [review]
Backout bug 887016
Review of attachment 8746072 [details] [diff] [review]:
-----------------------------------------------------------------
r=me to land on Aurora 48 only.
Attachment #8746072 -
Flags: review?(efaustbmo) → review+
Comment 31•8 years ago
|
||
Comment on attachment 8746074 [details] [diff] [review]
Backout bug 1263340 + small trivial changes to get it compiling
Review of attachment 8746074 [details] [diff] [review]:
-----------------------------------------------------------------
r=me to land on Aurora 48 only.
Attachment #8746074 -
Flags: review?(efaustbmo) → review+
Comment 32•8 years ago
|
||
Comment on attachment 8746075 [details] [diff] [review]
Fix FastInvokeGuard
Review of attachment 8746075 [details] [diff] [review]:
-----------------------------------------------------------------
r=me to land on Aurora 48 only.
Attachment #8746075 -
Flags: review?(efaustbmo) → review+
Assignee | ||
Comment 33•8 years ago
|
||
Assignee | ||
Comment 34•8 years ago
|
||
Between landing of the selfhosting and the backout, a patch has landed that moved every static function into a selfhosted function. Now with the backout I have to do that for split/replace/match/search. I did that mostly in the previous patches, but I forgot to add the selfhosted functions itself. This patch fixes this.
This is for aurora only!
Attachment #8747101 -
Flags: review?(till)
Comment 35•8 years ago
|
||
Comment on attachment 8747101 [details] [diff] [review]
last-issue
Review of attachment 8747101 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with less spitting :)
::: js/src/tests/ecma_6/RegExp/split-invalid-lastIndex.js
@@ +21,5 @@
> }
> };
> }
> };
> +if (Symbol.spit) {
Ideally, this would be "split", not "spit" :)
Attachment #8747101 -
Flags: review?(till) → review+
Assignee | ||
Comment 36•8 years ago
|
||
Rolled all patches into one. Carries over r+ from previous patches.
Approval Request Comment
[Feature/regressing bug #]:
Bug 887016
[User impact if declined]:
Possibility to more crashes and decrease in benchmark score on SS and kraken (wrt regexp/string matching. To stabilize we want to backout this on aurora in order to have an extra release to get all things fixed.
[Describe test coverage new/current, TreeHerder]:
Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13b330d92dc6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
This backout reverts to older well tested code.
[Risks and why]:
A lot of code has changed in between landing of bug 887016 and this backout. I tried my best to cover everything. But it is a risk that something wasn't backed out fully. Though I think the backout is safer than keeping this in tree currently. That is also the reason we want this in mozilla-aurora as soon as possible. To make sure that if that is the case, we can quickly react to it.
[String/UUID change made/needed]:
Only mdn will have to get adjusted. Which I will also do.
Attachment #8746072 -
Attachment is obsolete: true
Attachment #8746074 -
Attachment is obsolete: true
Attachment #8746075 -
Attachment is obsolete: true
Attachment #8747101 -
Attachment is obsolete: true
Attachment #8747420 -
Flags: review+
Attachment #8747420 -
Flags: approval-mozilla-aurora?
Comment 37•8 years ago
|
||
Comment on attachment 8747420 [details] [diff] [review]
Combined patch
Large backout from bug 887016 intended to improve stability.
This should land on aurora only.
Attachment #8747420 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi Wes, this is a much-needed fix for a top crasher on Aurora48. NI you just to be sure we land it today. Please and thank you! :)
status-firefox48:
--- → affected
Flags: needinfo?(wkocher)
Comment 39•8 years ago
|
||
failed to apply
applying rolledup
patching file js/src/builtin/Intl.js
Hunk #2 FAILED at 372
Hunk #3 FAILED at 831
2 out of 3 hunks FAILED -- saving rejects to file js/src/builtin/Intl.js.rej
patching file js/src/builtin/RegExp.js
Hunk #1 FAILED at 50
1 out of 3 hunks FAILED -- saving rejects to file js/src/builtin/RegExp.js.rej
Flags: needinfo?(hv1989)
Assignee | ||
Comment 40•8 years ago
|
||
I cannot set aurora-approval to + myself. But this is the same patch, except not bitrotten by the regexp backports that have landed recently.
Flags: needinfo?(hv1989)
Attachment #8748589 -
Flags: review+
Comment 41•8 years ago
|
||
Flags: needinfo?(wkocher)
Comment 42•8 years ago
|
||
Hannes, had to back this out seems this seem to have caused https://treeherder.mozilla.org/logviewer.html#?job_id=2527101&repo=mozilla-aurora
Flags: needinfo?(hv1989)
Comment 43•8 years ago
|
||
The test should just be removed.
Assignee | ||
Comment 44•8 years ago
|
||
Seems the rebase went wrong. And exactly on a spot that my local testing didn't want to show isues (Intl :( ). Using a try build this time to be sure.
Flags: needinfo?(hv1989)
Attachment #8748648 -
Flags: review+
Assignee | ||
Comment 45•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8748589 -
Attachment is obsolete: true
Assignee | ||
Comment 46•8 years ago
|
||
Comment 47•8 years ago
|
||
Since this was intended to land on Aurora only, marking as FIXED based on above landing.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 49•8 years ago
|
||
I see aurora talos improvements here (as expected):
https://treeherder.mozilla.org/perf.html#/alerts?id=1127
Assignee | ||
Comment 50•8 years ago
|
||
Updated the mdn pages from 48 to 49, according to this backout on aurora only.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/replace
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/search
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/split
https://developer.mozilla.org/en-US/Firefox/Releases/48
https://developer.mozilla.org/en-US/Firefox/Releases/49
https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_6_support_in_Mozilla
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/@@match
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/@@replace
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/@@search
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/@@split
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/@@species
You need to log in
before you can comment on or make changes to this bug.
Description
•