Closed Bug 1265307 Opened 9 years ago Closed 9 years ago

Delay selfhosting regexp and ES6 regexp conformity for a release

Categories

(Core :: JavaScript Engine: JIT, defect)

Product:
Core
Component:
JavaScript Engine: JIT
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(5 files, 5 obsolete files)

Perf numbers with backout patches
3.86 KB, text/plain
Details
Perf numbers before backout patches
3.77 KB, patch
Details | Diff | Splinter Review
Perf numbers before patch 887016 landed
4.33 KB, text/plain
Details
Combined patch
402.71 KB, patch
h4writer
: review+
lizzard
: approval-mozilla-aurora+
Details | Diff | Splinter Review
rolledup
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.
Depends on: 887016
Evilpie just mentioned merge day has been delayed for a week. As a result this bug too ;)
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)
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.
(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.
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)
> 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? :(
(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?
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.
(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.
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.
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!
Depends on: 1266676, 1260435, 1264264
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
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
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)
Opened bug 1267200 for the regexp regression on compare os
Depends on: 1267200
Flags: needinfo?(hv1989)
Attached patch Backout bug 887016 (obsolete) — — Splinter Review
Assignee: nobody → hv1989
Attachment #8746072 - Flags: review?(efaustbmo)
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)
Attached patch Fix FastInvokeGuard (obsolete) — — Splinter Review
Bug 1263558 renamed FastInvokeGuard to FastCallGuard. Fixing that.
Attachment #8746075 - Flags: review?(efaustbmo)
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)
please consider uplifting this to aurora (48) as we have regressions on there that will benefit from this.
Depends on: 1268056
(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 ;)
will there be a fix on trunk or other changes to address the perf regression this will fix on aurora?
(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 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 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 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+
Attached patch last-issue (obsolete) — — Splinter Review
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 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+
Attached patch Combined patch — — Splinter Review
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 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)
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)
Attached patch Combined patch - rebased (obsolete) — — Splinter Review
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+
Hannes, had to back this out seems this seem to have caused https://treeherder.mozilla.org/logviewer.html#?job_id=2527101&repo=mozilla-aurora
status-firefox48: fixed → affected
Flags: needinfo?(hv1989)
The test should just be removed.
Attached patch rolledup — — Splinter Review
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+
Attachment #8748589 - Attachment is obsolete: true
Since this was intended to land on Aurora only, marking as FIXED based on above landing.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
I see aurora talos improvements here (as expected): https://treeherder.mozilla.org/perf.html#/alerts?id=1127
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: