586544 - JM: Sync undefineds properly, or something

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

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

[Brendan Eich [:brendan]]

IIRC, Brian has a definite assignment analysis, with use-before-set detection, in the type inference analysis (bug 557407).

/be

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

He does, we were just talking about it at lunch!

Comment 3

[Brian Hackett [Laid off!]]

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.

Comment 4

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

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.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: WIP use-before-def patch

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.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: nfixed patch

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.

Comment 7

[Brendan Eich [:brendan]]

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

[Brendan Eich [:brendan]]

dvander, gal: IIRC we could use the analysis's results in TM too.

/be

Comment 9

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

Comment on attachment 466878 [details] [diff] [review]
nfixed patch

Sorry for the delay - good catch, thanks for fixing.

Comment 10

[Brian Hackett [Laid off!]]

http://hg.mozilla.org/projects/jaegermonkey/rev/fd11626b87c3

On AWFY looks like 4ms or so on SS, 100ms or so on V8.