Jon Coppeard (:jonco) (PTO until 14th September) Reporter
	Description • 9 years ago

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.

	Jon Coppeard (:jonco) (PTO until 14th September) Reporter
	Comment 1 • 9 years ago

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

	Jon Coppeard (:jonco) (PTO until 14th September) Reporter
	Comment 2 • 9 years ago

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

	Pulsebot
	Comment 3 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc263dfb7b13

	Jon Coppeard (:jonco) (PTO until 14th September) Reporter
	Comment 4 • 9 years ago

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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 5 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/cc263dfb7b13

	Luke Wagner [:luke]
	Comment 6 • 9 years ago

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()?

	Pulsebot
	Comment 7 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/999b9a743bed

	Carsten Book [:Tomcat]
	Comment 8 • 9 years ago

sorry had to back this out for hazard failures

	Pulsebot
	Comment 10 • 9 years ago

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

	Jon Coppeard (:jonco) (PTO until 14th September) Reporter
	Comment 11 • 9 years ago

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?

	Steve Fink [:sfink] [:s:] Assignee
	Comment 12 • 9 years ago

(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.)

	Steve Fink [:sfink] [:s:] Assignee
	Comment 13 • 9 years ago

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.

	Steve Fink [:sfink] [:s:] Assignee
	Comment 14 • 9 years ago

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. :-)

	Steve Fink [:sfink] [:s:] Assignee
	Comment 15 • 9 years ago

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.)

	Steve Fink [:sfink] [:s:] Assignee
	Comment 16 • 9 years ago

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>!

	Steve Fink [:sfink] [:s:] Assignee
	Comment 17 • 9 years ago

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

	Jon Coppeard (:jonco) (PTO until 14th September) Reporter
	Comment 18 • 9 years ago

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.

	Jon Coppeard (:jonco) (PTO until 14th September) Reporter
	Comment 19 • 9 years ago

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.

	Jon Coppeard (:jonco) (PTO until 14th September) Reporter
	Comment 20 • 9 years ago

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.

	Steve Fink [:sfink] [:s:] Assignee
	Comment 21 • 9 years ago

(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.)

	Pulsebot
	Comment 22 • 9 years ago

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

	Pulsebot
	Comment 23 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/cece92779e6e

	Pulsebot
	Comment 24 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9b2910c25f9

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 25 • 9 years ago

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