Closed
Bug 586544
Opened 14 years ago
Closed 14 years ago
JM: Sync undefineds properly, or something
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dvander, Assigned: bhackett1024)
References
Details
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
Right now we only sync one half of "undefined" - the type tag. The payload must be synced as well to make comparison with JSVAL_VOID work.
Syncing both would double the amount of stores in function prologues. For now, I'm spot fixing places where this breaks.
The plan for this bug is to back these changes out and do the right thing. If it's a perf regression, we can figure out something clever, like only write undef to slots that are known to be read before set.
Comment 1•14 years ago
|
||
IIRC, Brian has a definite assignment analysis, with use-before-set detection, in the type inference analysis (bug 557407).
/be
Comment 2•14 years ago
|
||
He does, we were just talking about it at lunch!
Assignee | ||
Comment 3•14 years ago
|
||
The is-local-defined analysis is pretty quick and comes before the actual type inference, so would be pretty easy to rip out. This would also give cross-branch/loop constant/copy propagation, give a place to hang register information for cross-branch/loop regalloc, and enable other static analysis like that in bug 584987.
Reporter | ||
Comment 4•14 years ago
|
||
Trying the dumb thing ended up being a pretty bad perf regression on v8 (3% or so) which I don't want to take. Talked to bhackett, we'll try to get his analysis in to augment/replace BytecodeAnalyzer.
Assignee | ||
Comment 5•14 years ago
|
||
This patch spins off the use-before-def analysis from the inference (modified somewhat) into a new jsanalyze file. Breakdown of local variables for SS/V8:
Accessible by eval SS: 45 V8: 0
These should be set to undefined at function entry. Primarily in date-format-tofte in SS, the Date.prototype.formatDate function (1000 calls).
Escapes via callee upvars SS: 3 V8: 250
These also need to be set to undefined at function entry. 194 of these are in v8-regexp.js, in functions called a paltry number of times. Another 54 are spread out in v8-earley-boyer's nasty autogenerated code, don't know how active these are.
Non-eval/escaping use-before-def SS: 3 V8: 16
Mostly in v8-regexp, again in functions called a paltry number of times.
Non-eval/escaping and def-before-use SS: 382 V8: 565
I suspect these are catching the hot locals in V8/SS, and we can't do better than this without deeper interprocedural information about when eval or an upvar-accessing function can be transitively called (aka type inference needed).
Other issues:
Analysis time is 1.6ms on SS, 1.8ms on V8, including the time spent computing BytecodeAnalyzer's information (stack depths, join points etc.). trace-tests and jstests pass, but there is a mysterious 8ms perf regression I need to track down (not analysis overhead, probably broken information produced by the analysis).
Also, this removes BytecodeAnalyzer.* and adds jsanalyze.* to the toplevel js/src. This was done because definitions are JIT-agnostic (e.g. the defined info may be useful to the tracer) and the data structures here should be used in the future to hang info for other analyses (i.e. type inference). No strong preference for where this data lives though.
Assignee: dvander → bhackett1024
Assignee | ||
Comment 6•14 years ago
|
||
This patch fixes a bug where undefined was written out to script->nslots, not script->nfixed. This also fixes stores of undefined to include the payload. This improves SS by 4ms for me, and V8 by 100ms.
Attachment #466878 -
Flags: review?(dvander)
Comment 7•14 years ago
|
||
Rockin'!
jsanalyze.cpp is fine for now, the intent to share the goodies is what is most important.
FYI, I proposed to sayrer and others recently that we clean up our source organization to have purty Analyzer.cpp and similar names, in an appropriate and not-deep directory structure, *before* we branch Firefox 4 and have to maintain branch and tip as heavily as I think we will.
So some time after September 1st, we'll finally be free of all the 8.3 Windows 3.1 hangover, jsmumblebarf.cpp names. I'll wiki a proposal.
/be
Comment 8•14 years ago
|
||
dvander, gal: IIRC we could use the analysis's results in TM too.
/be
Reporter | ||
Comment 9•14 years ago
|
||
Comment on attachment 466878 [details] [diff] [review]
nfixed patch
Sorry for the delay - good catch, thanks for fixing.
Attachment #466878 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 10•14 years ago
|
||
http://hg.mozilla.org/projects/jaegermonkey/rev/fd11626b87c3
On AWFY looks like 4ms or so on SS, 100ms or so on V8.
Reporter | ||
Updated•14 years ago
|
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•