Closed
Bug 739808
Opened 13 years ago
Closed 13 years ago
rm null closure no-clone optimization
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla14
People
(Reporter: dmandelin, Assigned: dmandelin)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
akeybl
:
approval-mozilla-esr10+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
Assignee: general → dmandelin
Attachment #609917 -
Flags: review?(luke)
![]() |
||
Comment 2•13 years ago
|
||
Comment on attachment 609917 [details] [diff] [review] Patch Rock
Attachment #609917 -
Flags: review?(luke) → review+
Assignee | ||
Comment 3•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/a09e61d9c648
Target Milestone: --- → mozilla14
Comment 4•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a09e61d9c648
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 5•13 years ago
|
||
Verified for trunk based on passing checked in tests since we have no manual cases / steps for this.
Status: RESOLVED → VERIFIED
Updated•13 years ago
|
tracking-firefox-esr10:
--- → 13+
tracking-firefox13:
--- → +
Updated•13 years ago
|
status-firefox14:
--- → fixed
Assignee | ||
Comment 6•13 years ago
|
||
Do you really want this for 13? It's a pretty big patch to backport.
Comment 7•13 years ago
|
||
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)?
Assignee | ||
Comment 8•13 years ago
|
||
[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
Attachment #623199 -
Flags: review?(luke)
Attachment #623199 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 9•13 years ago
|
||
[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.
Attachment #623218 -
Flags: review?(luke)
Attachment #623218 -
Flags: approval-mozilla-esr10?
![]() |
||
Comment 10•13 years ago
|
||
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.
Attachment #623199 -
Flags: review?(luke) → review+
![]() |
||
Comment 11•13 years ago
|
||
Comment on attachment 623218 [details] [diff] [review] Patch for ESR10 Same comment as above.
Attachment #623218 -
Flags: review?(luke) → review+
Updated•13 years ago
|
status-firefox-esr10:
--- → affected
status-firefox13:
--- → affected
Comment 12•13 years ago
|
||
Comment on attachment 623199 [details] [diff] [review] Patch for Fx13 [Triage Comment] Low risk fix - approved for Beta 13.
Attachment #623199 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 13•13 years ago
|
||
(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?
Assignee | ||
Comment 15•13 years ago
|
||
(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•13 years ago
|
||
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.
Attachment #623218 -
Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Assignee | ||
Comment 17•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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
Attachment #623218 -
Flags: approval-mozilla-esr10+
Updated•13 years ago
|
Updated•13 years ago
|
Attachment #623218 -
Flags: approval-mozilla-esr10?
Comment 22•13 years ago
|
||
There was flag confusion in bug 643967. Let's land on the ESR as soon as possible.
Updated•13 years ago
|
Attachment #623218 -
Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Comment 24•12 years ago
|
||
assumes the tinderboxes of various landings went green for verification per comment 15
You need to log in
before you can comment on or make changes to this bug.
Description
•