David Mandelin [:dmandelin] Assignee
	Description • 13 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.

	Luke Wagner [:luke]
	Comment 2 • 13 years ago

Comment on attachment 609917 [details] [diff] [review]
Patch

Rock

	David Mandelin [:dmandelin] Assignee
	Comment 3 • 13 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/a09e61d9c648

	Ed Morley [:emorley]
	Comment 4 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/a09e61d9c648

	Al Billings [:abillings - ex-MoCo]
	Comment 5 • 13 years ago

Verified for trunk based on passing checked in tests since we have no manual cases / steps for this.

	David Mandelin [:dmandelin] Assignee
	Comment 6 • 13 years ago

Do you really want this for 13? It's a pretty big patch to backport.

	Al Billings [:abillings - ex-MoCo]
	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)?

	David Mandelin [:dmandelin] 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

	David Mandelin [:dmandelin] 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.

	Luke Wagner [:luke]
	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.

	Luke Wagner [:luke]
	Comment 11 • 13 years ago

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

Same comment as above.

	Alex Keybl [:akeybl]
	Comment 12 • 13 years ago

Comment on attachment 623199 [details] [diff] [review]
Patch for Fx13

[Triage Comment]
Low risk fix - approved for Beta 13.

	Alex Keybl [:akeybl]
	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?

	David Mandelin [:dmandelin] Assignee
	Comment 14 • 13 years ago

http://hg.mozilla.org/releases/mozilla-beta/rev/6f0397098e05

	David Mandelin [:dmandelin] 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.

	Lukas Blakk [:lsblakk] use ?needinfo
	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.

	David Mandelin [:dmandelin] 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?

	Lukas Blakk [:lsblakk] use ?needinfo
	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.

	Lukas Blakk [:lsblakk] use ?needinfo
	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

	Lukas Blakk [:lsblakk] use ?needinfo
	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.

	Lukas Blakk [:lsblakk] use ?needinfo
	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

	Alex Keybl [:akeybl]
	Comment 22 • 13 years ago

There was flag confusion in bug 643967. Let's land on the ESR as soon as possible.

	David Mandelin [:dmandelin] Assignee
	Comment 23 • 13 years ago

http://hg.mozilla.org/releases/mozilla-esr10/rev/6cf09fa77444

	Tracy Walker [:tracy]
	Comment 24 • 12 years ago

assumes the tinderboxes of various landings went green for verification per comment 15