rm null closure no-clone optimization

VERIFIED FIXED in Firefox 13

Status

()

Core
JavaScript Engine
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: dmandelin, Assigned: dmandelin)

Tracking

(Blocks: 1 bug)

Trunk
mozilla14
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13+ verified, firefox14 verified, firefox-esr1013+ verified)

Details

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
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)

Updated

5 years ago
Blocks: 725161
(Assignee)

Comment 1

5 years ago
Created attachment 609917 [details] [diff] [review]
Patch
Assignee: general → dmandelin
Attachment #609917 - Flags: review?(luke)

Comment 2

5 years ago
Comment on attachment 609917 [details] [diff] [review]
Patch

Rock
Attachment #609917 - Flags: review?(luke) → review+
(Assignee)

Comment 3

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/a09e61d9c648
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/a09e61d9c648
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 643967
Verified for trunk based on passing checked in tests since we have no manual cases / steps for this.
Status: RESOLVED → VERIFIED
tracking-firefox-esr10: --- → 13+
tracking-firefox13: --- → +
status-firefox14: --- → fixed
(Assignee)

Comment 6

5 years ago
Do you really want this for 13? It's a pretty big patch to backport.
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

5 years ago
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
Attachment #623199 - Flags: review?(luke)
Attachment #623199 - Flags: approval-mozilla-beta?
(Assignee)

Comment 9

5 years ago
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.
Attachment #623218 - Flags: review?(luke)
Attachment #623218 - Flags: approval-mozilla-esr10?

Comment 10

5 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

5 years ago
Comment on attachment 623218 [details] [diff] [review]
Patch for ESR10

Same comment as above.
Attachment #623218 - Flags: review?(luke) → review+
status-firefox-esr10: --- → affected
status-firefox13: --- → affected
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+
(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 14

5 years ago
http://hg.mozilla.org/releases/mozilla-beta/rev/6f0397098e05
status-firefox13: affected → fixed
(Assignee)

Comment 15

5 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 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

5 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?
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.
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
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 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+
tracking-firefox-esr10: 13+ → 14+
Attachment #623218 - Flags: approval-mozilla-esr10?
There was flag confusion in bug 643967. Let's land on the ESR as soon as possible.
tracking-firefox-esr10: 14+ → 13+

Updated

5 years ago
Attachment #623218 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
(Assignee)

Comment 23

5 years ago
http://hg.mozilla.org/releases/mozilla-esr10/rev/6cf09fa77444
status-firefox-esr10: affected → fixed
assumes the tinderboxes of various landings went green for verification per comment 15
status-firefox-esr10: fixed → verified
status-firefox13: fixed → verified
status-firefox14: fixed → verified
You need to log in before you can comment on or make changes to this bug.