697861 - Remove defined variables analysis

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

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 [Laid off!]]

Attachment: patch (2b0d447b1b9f)

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 570106 [details] [diff] [review]
patch (2b0d447b1b9f)

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

Nice cleanup.

Comment 3

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa953731b2c6

Comment 4

[Ed Morley [:emorley]]

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

Comment 5

[Brian Hackett [Laid off!]]

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

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 [Laid off!]]

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

Could we investigate to see which part fixes the freeze and try to come up with a minimal patch?

Comment 9

[Brian Hackett [Laid off!]]

Attachment: 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]]

(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 [Laid off!]]

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

Comment 12

[Alex Keybl [:akeybl]]

(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 [Laid off!]]

(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]]

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.