Last Comment Bug 697861 - Remove defined variables analysis
: Remove defined variables analysis
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 All
: -- normal (vote)
: mozilla10
Assigned To: Brian Hackett (:bhackett)
:
Mentors:
: 707950 (view as bug list)
Depends on:
Blocks: 689284 707950
  Show dependency treegraph
 
Reported: 2011-10-27 15:39 PDT by Brian Hackett (:bhackett)
Modified: 2011-12-16 06:47 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2b0d447b1b9f) (29.02 KB, patch)
2011-10-27 15:40 PDT, Brian Hackett (:bhackett)
dvander: review+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review
potential minimized fix (4.21 KB, patch)
2011-12-07 13:28 PST, Brian Hackett (:bhackett)
no flags Details | Diff | Splinter Review

Description Brian Hackett (:bhackett) 2011-10-27 15:39:35 PDT
The analysis done in the first pass of ScriptAnalysis to identify variables defined at each point is subsumed by SSA analysis and barely used anymore anyways.  Additionally, it has a cheesy limit of 50 variables after which it marks all variables as having a use-before-def, which is in place because it doesn't scale well as the number of variables gets large (as opposed to SSA, which at least *should* be able to scale in such circumstances).
Comment 1 Brian Hackett (:bhackett) 2011-10-27 15:40:34 PDT
Created attachment 570106 [details] [diff] [review]
patch (2b0d447b1b9f)
Comment 2 David Anderson [:dvander] 2011-10-28 15:57:28 PDT
Comment on attachment 570106 [details] [diff] [review]
patch (2b0d447b1b9f)

Review of attachment 570106 [details] [diff] [review]:
-----------------------------------------------------------------

Nice cleanup.
Comment 3 Brian Hackett (:bhackett) 2011-10-29 17:30:46 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa953731b2c6
Comment 4 Ed Morley [:emorley] 2011-10-30 10:33:17 PDT
https://hg.mozilla.org/mozilla-central/rev/aa953731b2c6
Comment 5 Brian Hackett (:bhackett) 2011-12-07 12:24:32 PST
Comment on attachment 570106 [details] [diff] [review]
patch (2b0d447b1b9f)

This patch was identified in bug 707950 as fixing a site freeze.  It is already in aurora.
Comment 6 Alex Keybl [:akeybl] 2011-12-07 12:36:49 PST
Is this entire patch necessary to fix the issue in 707950? Are there any lower risk options that we can take a look at instead (perhaps even a Mac specific fix)?
Comment 7 Brian Hackett (:bhackett) 2011-12-07 12:42:21 PST
This depends on whether we can identify what is causing the freeze.  The patch is doing two things: removing a bunch of complexity from the bytecode analysis done during compilation, and replacing the single use of the information that had been generated by that complexity.  If the freeze is happening in jitcode or somewhere nearby, the latter portion could be done in isolation, which would require applying just the portion of the patch which modifies js/src/methodjit/Compiler.*

I don't think this is a mac-specific problem.
Comment 8 Alex Keybl [:akeybl] 2011-12-07 13:24:22 PST
Could we investigate to see which part fixes the freeze and try to come up with a minimal patch?
Comment 9 Brian Hackett (:bhackett) 2011-12-07 13:28:51 PST
Created attachment 579815 [details] [diff] [review]
potential minimized fix

This is a potential minimal fix.  Can someone who has been able to repro the freeze apply this to beta tip and try the site?
Comment 10 Alex Keybl [:akeybl] 2011-12-07 14:51:15 PST
(In reply to Brian Hackett (:bhackett) from comment #9)
> Created attachment 579815 [details] [diff] [review] [diff] [details] [review]
> potential minimized fix
> 
> This is a potential minimal fix.  Can someone who has been able to repro the
> freeze apply this to beta tip and try the site?

Can we get a try build with the current beta branch plus this patch? The URL in https://bugzilla.mozilla.org/show_bug.cgi?id=707950#c0 worked for me, so let's just use that to test.
Comment 11 Brian Hackett (:bhackett) 2011-12-07 16:42:10 PST
https://tbpl.mozilla.org/?tree=Try&rev=6decebc292e9
Comment 12 Alex Keybl [:akeybl] 2011-12-07 23:21:42 PST
(In reply to Brian Hackett (:bhackett) from comment #11)
> https://tbpl.mozilla.org/?tree=Try&rev=6decebc292e9

Just tested this - looks like it didn't fix the issue of hanging on https://bugzilla.mozilla.org/show_bug.cgi?id=707950#c0.

Do you have any more ideas of what within this patch may be causing the freeze? Let us know if you think it's lower risk to take the whole patch at any point, in which case can you speak to the risk of doing that?
Comment 13 Brian Hackett (:bhackett) 2011-12-08 08:41:00 PST
(In reply to Alex Keybl [:akeybl] from comment #12)
> (In reply to Brian Hackett (:bhackett) from comment #11)
> > https://tbpl.mozilla.org/?tree=Try&rev=6decebc292e9
> 
> Just tested this - looks like it didn't fix the issue of hanging on
> https://bugzilla.mozilla.org/show_bug.cgi?id=707950#c0.
> 
> Do you have any more ideas of what within this patch may be causing the
> freeze? Let us know if you think it's lower risk to take the whole patch at
> any point, in which case can you speak to the risk of doing that?

The rest of the patch is just deleting code, so this site is probably exposing a bug in the code which was deleted.  I think it would be lower risk to just take the whole patch, rather than try to figure out which piece is malfunctioning.
Comment 14 Alex Keybl [:akeybl] 2011-12-08 14:46:48 PST
Comment on attachment 570106 [details] [diff] [review]
patch (2b0d447b1b9f)

[Triage Comment]
We've decided to be risk averse here and let this ride the train since this patch is larger than a fix and we're one beta away from release. Minusing for beta.
Comment 15 Robert Kaiser 2011-12-16 06:47:09 PST
*** Bug 707950 has been marked as a duplicate of this bug. ***

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