Last Comment Bug 739808 - rm null closure no-clone optimization
: rm null closure no-clone optimization
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla14
Assigned To: David Mandelin [:dmandelin]
:
Mentors:
Depends on:
Blocks: IonMonkey 643967 725161
  Show dependency treegraph
 
Reported: 2012-03-27 15:26 PDT by David Mandelin [:dmandelin]
Modified: 2012-07-12 10:32 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
verified
13+
verified


Attachments
Patch (135.57 KB, patch)
2012-03-27 15:28 PDT, David Mandelin [:dmandelin]
luke: review+
Details | Diff | Review
Patch for Fx13 (12.11 KB, patch)
2012-05-11 10:21 PDT, David Mandelin [:dmandelin]
luke: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review
Patch for ESR10 (10.62 KB, patch)
2012-05-11 10:57 PDT, David Mandelin [:dmandelin]
luke: review+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Review

Description David Mandelin [:dmandelin] 2012-03-27 15:26:36 PDT
The no-clone optimization and the method barrier are gonna make life hard for IonMonkey, because they'd have to implement some form of getValidCalleeObject. Also, previous measurements showed that in practice we only get a small memory savings out of it, which doesn't really pay back for the complexity. Patch coming shortly.
Comment 1 David Mandelin [:dmandelin] 2012-03-27 15:28:08 PDT
Created attachment 609917 [details] [diff] [review]
Patch
Comment 2 Luke Wagner [:luke] 2012-03-27 15:36:43 PDT
Comment on attachment 609917 [details] [diff] [review]
Patch

Rock
Comment 3 David Mandelin [:dmandelin] 2012-03-27 17:16:06 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/a09e61d9c648
Comment 4 Ed Morley [:emorley] 2012-03-28 14:01:51 PDT
https://hg.mozilla.org/mozilla-central/rev/a09e61d9c648
Comment 5 [On PTO until 6/29] 2012-04-10 13:45:49 PDT
Verified for trunk based on passing checked in tests since we have no manual cases / steps for this.
Comment 6 David Mandelin [:dmandelin] 2012-05-02 18:01:25 PDT
Do you really want this for 13? It's a pretty big patch to backport.
Comment 7 [On PTO until 6/29] 2012-05-03 09:36:29 PDT
Given what it blocks, we should take it if we can. How risky is it to try (other than the obvious since it is huge)?
Comment 8 David Mandelin [:dmandelin] 2012-05-11 10:21:53 PDT
Created attachment 623199 [details] [diff] [review]
Patch for Fx13

[Approval Request Comment]
Regression caused by (bug #): null closure no-clone optimization
User impact if declined: instability from a fragile optimization
Testing completed (on m-c, etc.): shell tests + try + m-c for the larger patch
Risk to taking this patch (and alternatives if risky): low: could theoretically increase memory usage on some sites, but failed to observe this with the larger patch; it's conceivable that some of the nonremoved code could somehow execute but I verified not in shell tests.
String changes made by this patch: none
Comment 9 David Mandelin [:dmandelin] 2012-05-11 10:57:54 PDT
Created attachment 623218 [details] [diff] [review]
Patch for ESR10

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: was requested
User impact if declined: same as beta
Fix Landed on Version: m-c
Risk to taking this patch (and alternatives if risky): a little more risk than beta: in ESR10, the null closure optimization is intertwined with branding (an optimization that was subsequently removed), which I discovered by running shell tests. I think I fixed the problem but it's hard to be sure
String changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Comment 10 Luke Wagner [:luke] 2012-05-11 11:38:31 PDT
Comment on attachment 623199 [details] [diff] [review]
Patch for Fx13

It would be nice to remove 'setJoinable' and the associated flag it sets to statically enforce that it doesn't ever get set.
Comment 11 Luke Wagner [:luke] 2012-05-11 11:39:25 PDT
Comment on attachment 623218 [details] [diff] [review]
Patch for ESR10

Same comment as above.
Comment 12 Alex Keybl [:akeybl] 2012-05-11 16:34:18 PDT
Comment on attachment 623199 [details] [diff] [review]
Patch for Fx13

[Triage Comment]
Low risk fix - approved for Beta 13.
Comment 13 Alex Keybl [:akeybl] 2012-05-11 16:34:56 PDT
(In reply to David Mandelin from comment #9)
> Risk to taking this patch (and alternatives if risky): a little more risk
> than beta: in ESR10, the null closure optimization is intertwined with
> branding (an optimization that was subsequently removed), which I discovered
> by running shell tests. I think I fixed the problem but it's hard to be sure

What sort of QA testing or automated testing can we do with the ESR to verify that there aren't any related regressions once landed?
Comment 14 David Mandelin [:dmandelin] 2012-05-14 17:01:31 PDT
http://hg.mozilla.org/releases/mozilla-beta/rev/6f0397098e05
Comment 15 David Mandelin [:dmandelin] 2012-05-14 17:09:15 PDT
(In reply to Alex Keybl [:akeybl] from comment #13)
> (In reply to David Mandelin from comment #9)
> > Risk to taking this patch (and alternatives if risky): a little more risk
> > than beta: in ESR10, the null closure optimization is intertwined with
> > branding (an optimization that was subsequently removed), which I discovered
> > by running shell tests. I think I fixed the problem but it's hard to be sure
> 
> What sort of QA testing or automated testing can we do with the ESR to
> verify that there aren't any related regressions once landed?

Tinderbox coverage is usually pretty good for this sort of thing. The ESR patch passes the jit-tests, I'm now running it on try. Otherwise, there's not too much, because if there is a problem, it would probably be from a subtle behavioral change involving function-valued properties of JS objects created by object literals, and I don't see any particularly easy way to test for that.
Comment 16 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-15 16:56:48 PDT
Comment on attachment 623218 [details] [diff] [review]
Patch for ESR10

Approved for landing contingent on a successful try run. If you can paste in your try run on completion before landing so we can see the tinderbox results that would be good to have here.
Comment 17 David Mandelin [:dmandelin] 2012-05-22 18:51:27 PDT
Here's that try run.

https://tbpl.mozilla.org/?tree=Try&rev=e8659d93046a

The red seems to be unrelated to this patch. Does it look good?
Comment 18 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-30 15:46:42 PDT
looked into it and we use different mozconfigs in esr than in m-c so try isn't really usable for esr builds.  the error isn't related to your patch so please go ahead with landing.
Comment 19 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-30 15:47:13 PDT
fwiw, we can double check that the OS X builds work on http://tbpl.mozilla.org/?tree=Mozilla-Esr10 once you've landed just to be safe
Comment 20 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-30 15:50:27 PDT
Sorry, I take it back - bug 643967 has now been moved to track for 14+ so I'm moving this bug to that release as well.  Please do not land this patch on the current mozilla-esr10 repo, resetting approval flags as well.
Comment 21 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-30 15:51:00 PDT
Comment on attachment 623218 [details] [diff] [review]
Patch for ESR10

removing approval to ensure we do not land this in the release going with Firefox 13
Comment 22 Alex Keybl [:akeybl] 2012-05-30 17:42:29 PDT
There was flag confusion in bug 643967. Let's land on the ESR as soon as possible.
Comment 23 David Mandelin [:dmandelin] 2012-05-30 18:37:23 PDT
http://hg.mozilla.org/releases/mozilla-esr10/rev/6cf09fa77444
Comment 24 Tracy Walker [:tracy] 2012-07-12 10:32:27 PDT
assumes the tinderboxes of various landings went green for verification per comment 15

Note You need to log in before you can comment on or make changes to this bug.