Closed
Bug 697861
Opened 13 years ago
Closed 13 years ago
Remove defined variables analysis
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
Details
Attachments
(2 files)
29.02 KB,
patch
|
dvander
:
review+
akeybl
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
4.21 KB,
patch
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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•13 years ago
|
||
Comment 4•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Assignee | ||
Comment 5•13 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?
Comment 6•13 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•13 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•13 years ago
|
||
Could we investigate to see which part fixes the freeze and try to come up with a minimal patch?
Assignee | ||
Comment 9•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
Comment 12•13 years ago
|
||
(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•13 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 14•13 years ago
|
||
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-
You need to log in
before you can comment on or make changes to this bug.
Description
•