Remove defined variables analysis

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

(Blocks: 1 bug)

unspecified
mozilla10
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

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

Comment 1

6 years ago
Created attachment 570106 [details] [diff] [review]
patch (2b0d447b1b9f)
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+
(Assignee)

Comment 3

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

Comment 4

6 years ago
https://hg.mozilla.org/mozilla-central/rev/aa953731b2c6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
(Assignee)

Comment 5

6 years ago
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?
(Assignee)

Updated

6 years ago
Blocks: 707950

Comment 6

6 years ago
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)?
(Assignee)

Comment 7

6 years ago
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

6 years ago
Could we investigate to see which part fixes the freeze and try to come up with a minimal patch?
(Assignee)

Comment 9

6 years ago
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?
(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
(Assignee)

Comment 11

6 years ago
https://tbpl.mozilla.org/?tree=Try&rev=6decebc292e9
(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?
(Assignee)

Comment 13

6 years ago
(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-

Updated

6 years ago
Duplicate of this bug: 707950
You need to log in before you can comment on or make changes to this bug.