Currently there are three functions in frontend/BytecodeCompiler.cpp that are take source as input and compile it to bytecode - CompileScript(), CompileLazyFunction() and CompileFunctionBody() - and there will soon be a fourth to compile modules. These functions are all pretty monolithic and share much code, although they also have many differences. This bug is to try to find a way to refactor this to allow adding CompileModule() without duplicating more code.

The first thing to notice is that CompileLazyFunction() is pretty different to the others and doesn't contain much common code, so I'm going to leave that. The others have the following common steps that could be factored out: - create script source - source code compression - create syntax parser - create full parser They also have these steps which although common aren't so easy to factor: - a loop to retry parsing if syntax parsing fails, which for scripts also compiles one statement at a time - creation of a bytecode emitter which happens before parsing for scripts and afterwards for function bodies

As a first step we can factor out the tracelogger code.

This patch adds a BytecodeCompiler class to hold common state and splits most of the compiler work into separate methods which can then be shared. This roughly halves the size of compileScript() and compileFunctionBody(). This should all be code motion with no semantic changes. The changes ended up being more drastic than expected, but hopefully this is better that what we had before.

Comment on attachment 8632760 [details] [diff] [review] refactor-bytecode-compilation Review of attachment 8632760 [details] [diff] [review]: ----------------------------------------------------------------- Nice refactoring. ::: js/src/frontend/BytecodeCompiler.cpp @@ +548,5 @@ > > +bool > +BytecodeCompiler::finish() > +{ > + return !maybeSourceCompressor || maybeSourceCompressor->complete(); For visual symmetry with maybeCompressSource(), perhaps rename this to maybeCompleteCompressSource(). @@ +551,5 @@ > +{ > + return !maybeSourceCompressor || maybeSourceCompressor->complete(); > +} > + > +JSScript* BytecodeCompiler::compileScript(HandleObject scopeChain, HandleScript evalCaller, \n after "JScript*" @@ +556,5 @@ > + unsigned staticLevel) > +{ > + if (!createScriptSource() || > + !maybeCompressSource() || > + !createParser()) These 3 appear together in all cases so perhaps you could fold them into one createSourceAndParser()?

sorry had to back this out for hazard failures

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/d693dbecbaaf

So the hazard analysis thinks the new BytecodeCompiler stack class is a GC pointer for several reasons: GCPointer: BytecodeCompiler contains field 'script' of type class JS::Rooted<JSScript*> contains field 'ptr' pointing to type JSScript which is a GCThing because I said so contains field 'startPosition' of type js::frontend::TokenStream::Position contains field 'currentToken' of type js::frontend::Token contains field 'u' of type js::frontend::Token:u contains field 'name' pointing to type js::PropertyName contains field 'field:0' of type JSAtom (probably via inheritance) contains field 'field:0' of type JSFlatString (probably via inheritance) contains field 'field:0' of type JSLinearString (probably via inheritance) contains field 'field:0' of type JSString (probably via inheritance) which is a GCThing because I said so contains field 'atom' pointing to type JSAtom contains field 'lookaheadTokens' of type js::frontend::Token contains field 'sourceObject' of type class JS::Rooted<js::ScriptSourceObject*> contains field 'ptr' pointing to type js::ScriptSourceObject contains field 'field:0' of type js::NativeObject (probably via inheritance) contains field 'field:0' of type JSObject (probably via inheritance) which is a GCThing because I said so contains field 'slots_' pointing to type js::HeapSlot contains field 'field:0' of type class js::BarrieredBase<JS::Value> (probably via inheritance) contains field 'value' of type JS::Value which is a GCPointer because I said so contains field 'elements_' pointing to type js::HeapSlot contains field 'shape_' of type class js::HeapPtr<js::Shape*> contains field 'field:0' of type class js::BarrieredBase<js::Shape*> (probably via inheritance) contains field 'value' pointing to type js::Shape which is a GCThing because I said so contains field 'enclosingStaticScope' of type class JS::Rooted<js::ScopeObject*> contains field 'ptr' pointing to type js::ScopeObject contains field 'field:0' of type js::NativeObject (probably via inheritance) The 'startPosition' field I understand - we contain something that has a JSAtom*. This is actually safe because we also contain an AutoKeepAtoms instance but the analysis doesn't know this. I don't understand why containing a Rooted<T> is a reason though. Other classes (e.g. CompileOptions) do this and don't get treated as GC pointers. Steve, do you know why this is?

(In reply to Jon Coppeard (:jonco) from comment #11) > So the hazard analysis thinks the new BytecodeCompiler stack class is a GC > pointer for several reasons: > > The 'startPosition' field I understand - we contain something that has a > JSAtom*. This is actually safe because we also contain an AutoKeepAtoms > instance but the analysis doesn't know this. > > I don't understand why containing a Rooted<T> is a reason though. Other > classes (e.g. CompileOptions) do this and don't get treated as GC pointers. > Steve, do you know why this is? You are correct. That's a bug. But it *might* only be a bug in the gcTypes.txt reporting? As in, it may be computing the set of types correctly (and the JSAtom* correctly catches this one), but reporting the reasons incorrectly. Which is kind of weird, because it records the reasons while it's computing the set of types in the first place. So I'll need to look at this, but hopefully if you clear up the JSAtom* one then it'll work for your case? Filed bug 1184199 for this. If it turns out that "fixing" the JSAtom* does not clear this up, then you can mark it as blocking this bug. (Quotes around fixing given that JSAtoms don't currently need to be rooted in the first place.)

Sorry, this is really two changes in one. It renames depth -> typePtrLevel (the ptrdness of the typeName parameter) and ptrdness -> fieldPtrLevel (the ptrdness of the 'child' field parameter). But it also fixes places where the child,why were swapped (child == field name, why == field type, and I probably ought to rename those too.) That causes the isRooted* calls to be applied to the right things, which fixes the bogohazard here (where BytecodeCompiler was incorrectly believed to be a GCPointer.) At least, I *think* it was this patch that fixed it. It might have been a later one.

This patch should look familiar. I used different names, but it's very similar to what you posted to the other bug. Although it doesn't touch analyzeRoots.js, it changes what it's doing by redefining isRootedTypeName to accept *any* rooted type, regardless of ptrdness. That causes more things to flow into the "unnecessary roots" branch, but otherwise has little effect -- in order to be considered a hazard, a variable type must pass the isUnrootedType check, and that does not overlap with the isRootedType check. In summary, this adds in some missing "unnecessary roots" reports, which we never look at anyway. Oh, and it means the code sees more tyeps of code, which broke a couple of things and I just realized my patch stack is in the wrong order. I want to land fixes before breakages. :-)

This fixes the breakage in the patch I just posted. (I've reordered them in my stack, so I'll land in the right order.) As a result of passing more types through isRootedType, the analysis encountered several code patterns that previously had not been seen and caused assert failures in edgeCanGC. So this generalizes edgeCanGC to handle them. (They're all cases where we have function pointers in args, locals, or temporaries.)

Adding things like TypeSet::Type to the set of GCPointers is going to introduce two hazards. This fixes them. ('elementType' and 'key' are both live across a GC.) The new rooting infrastructure was very helpful here, especially since Terrence just un-implemented Rooted<TypeSet::Type>!

Annotate TypeSet stuff. My local shell-only run, at least, has no hazards with the previous patch and this one applied.

Comment on attachment 8636878 [details] [diff] [review] Improve naming Review of attachment 8636878 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/devtools/rootAnalysis/computeGCTypes.js @@ +85,2 @@ > { > + //printErr(`${indent}${typeName}${stars(typePtrLevel)} may be a gctype/ptr because of its child '${child}' of type ${why}${stars(fieldPtrLevel)}`); You probably meant to remove this.

Comment on attachment 8636881 [details] [diff] [review] Recognize more rooted type names Review of attachment 8636881 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: js/src/devtools/rootAnalysis/annotations.js @@ +271,5 @@ > +{ > + if (name == "JSAddonId") > + { > + return true; > + } Nit: braces not needed for single line if statement.

Comment on attachment 8636882 [details] [diff] [review] Accept more edge types in edgeCanGC Review of attachment 8636882 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/devtools/rootAnalysis/analyzeRoots.js @@ +285,2 @@ > var callee = edge.Exp[0]; > + while (true) { Would it be simpler to do this? while (callee.Kind == "Drf") callee = callee.Exp[0]; @@ +298,5 @@ > + return indirectCallCannotGC(functionName, varName) ? null : "*" + varName; > + } > + > + if (callee.Kind == "Fld") { > + //var field = callee.Exp[0].Field; Please remove commented out code. @@ +301,5 @@ > + if (callee.Kind == "Fld") { > + //var field = callee.Exp[0].Field; > + var field = callee.Field; > + if (!field) > + printErr(JSON.stringify(callee, null, 4)); Should probably print a meaningful error here.

(In reply to Jon Coppeard (:jonco) from comment #20) > Comment on attachment 8636882 [details] [diff] [review] > Accept more edge types in edgeCanGC > > Review of attachment 8636882 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/devtools/rootAnalysis/analyzeRoots.js > @@ +285,2 @@ > > var callee = edge.Exp[0]; > > + while (true) { > > Would it be simpler to do this? > > while (callee.Kind == "Drf") > callee = callee.Exp[0]; Um... yes. But only an idiot wouldn't have seen that. Are you calling me an idiot? (Fixed, thanks.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/237d71f50a16 https://hg.mozilla.org/integration/mozilla-inbound/rev/15ee99905e6d https://hg.mozilla.org/integration/mozilla-inbound/rev/1805b2713be9 https://hg.mozilla.org/integration/mozilla-inbound/rev/d08ebf664561 https://hg.mozilla.org/integration/mozilla-inbound/rev/0599431ae5b1

https://hg.mozilla.org/mozilla-central/rev/237d71f50a16 https://hg.mozilla.org/mozilla-central/rev/15ee99905e6d https://hg.mozilla.org/mozilla-central/rev/1805b2713be9 https://hg.mozilla.org/mozilla-central/rev/d08ebf664561 https://hg.mozilla.org/mozilla-central/rev/0599431ae5b1 https://hg.mozilla.org/mozilla-central/rev/cece92779e6e https://hg.mozilla.org/mozilla-central/rev/e9b2910c25f9