Closed Bug 697861 Opened 8 years ago Closed 8 years ago

Remove defined variables analysis

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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).
Assignee: general → bhackett1024
Attachment #570106 - Flags: review?(dvander)
Comment on attachment 570106 [details] [diff] [review]
patch (2b0d447b1b9f)

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

Nice cleanup.
Attachment #570106 - Flags: review?(dvander) → review+
https://hg.mozilla.org/mozilla-central/rev/aa953731b2c6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
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.
Attachment #570106 - Flags: approval-mozilla-beta?
Blocks: 707950
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)?
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.
Could we investigate to see which part fixes the freeze and try to come up with a minimal patch?
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?
(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.
OS: Mac OS X → All
(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?
(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 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.
Attachment #570106 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Duplicate of this bug: 707950
You need to log in before you can comment on or make changes to this bug.