1401243 - Remove AutoGCRooter for Parser and BinParser

Status: RESOLVED DUPLICATE of bug 1634435

(Core :: JavaScript: GC, enhancement, P3)

Description

[Steve Fink [:sfink] [:s:]]

AutoGCRooter contains a case for Parser, which is not really necessary. Parser actually has a trace() method already and could be trivially used within Rooted<Parser*>, except that the actual usage of Parser is to create one on the stack. Yoric is adding another AutoGCRooter for BinParser.

One way would be to do

  Parser p(...);
  Rooted<Parser*> root(cx, &p);

I *think* that would work, even though it looks very wrong. (The Rooted<> would just be responsible for tracing the Parser; the Parser's address itself would never change.)

I think both could probably use a PersistentRooted member field instead.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Remove AutoGCRooter for Parser

What do you think, Jon? Is there a simpler way to do this?

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #1)
The ideal way would be to do:

  Rooted<Parser> parser(...);

But then to make this work nicely you would have implement WrappedPtrOperations for the Parser interface which would be massive overkill.  Or perhaps you wouldn't need to do all of it?

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 8909880 [details] [diff] [review]
Remove AutoGCRooter for Parser

Review of attachment 8909880 [details] [diff] [review]:
-----------------------------------------------------------------

This is a bit strange but better than what we have already so I'm going to r+.

::: js/src/frontend/Parser.h
@@ +511,5 @@
> +        Parser* parser;
> +        Rooter(Parser* parser) : parser(parser) {}
> +        void trace(JSTracer* trc) { parser->trace(trc); }
> +    };
> +    PersistentRooted<Rooter> root;

I think this can just be Rooted<> here.  But I'm not sure there's much in it.

There should be a comment somewhere explaining that this roots itself.

Comment 4

[Steve Fink [:sfink] [:s:]]

Rooted does not work. It's a little weird, because a Parser may contain another Parser (in syntaxParser_). It is only assigned in the constructor, so it seems like it might almost be ok, except that BytecodeCompiler.cpp creates both parsers via emplace(), and I suspect it's doing them out of order (because I tried switching to Rooted and wrote up an explanation for why that should be fine, then tried running and got an immediate crash.)

They're stored in Maybe<Parser> fields, and even though it might work if I reorder them, it just feels too brittle to depend on. It would be reasonable for a caller to heap-allocate the syntax parser and use it for multiple parsers, or something crazy like that.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c7dfc3cc0c0
Remove AutoGCRooter for Parser, r=jonco

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f668fa4b566d
followup - fix explicit constructor warning on a CLOSED TREE

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/269c3306bcfd
Backed out changeset f668fa4b566d 
https://hg.mozilla.org/integration/mozilla-inbound/rev/35e80f56a797
Backed out changeset 3c7dfc3cc0c0 on request from sfink (issue with hazard build). r=backout on a CLOSED TREE

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out on request from sfink (issue with hazard build). Please also keep an eye on the follow-up push for more test failures.

https://hg.mozilla.org/integration/mozilla-inbound/rev/35e80f56a79726c4c79b559a07e43324264a3b61
https://hg.mozilla.org/integration/mozilla-inbound/rev/269c3306bcfd55fcbc757995d763088fdcf573ab

Follow-up push with failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/f668fa4b566d4cfe2ddcda84989d7c2df48eac9d
Failure log hazard: https://treeherder.mozilla.org/logviewer.html#?job_id=132251069&repo=mozilla-inbound

Function '_ZL11SyntaxParseP9JSContextjPN2JS5ValueE$js.cpp:uint8 SyntaxParse(JSContext*, uint32, JS::Value*)' has unrooted 'parser' of type 'class js::frontend::Parser<js::frontend::SyntaxParseHandler, char16_t>' live across GC call '_ZN2js8frontend6ParserINS0_18SyntaxParseHandlerEDsE12checkOptionsEv$bool js::frontend::Parser<ParseHandler, CharT>::checkOptions() [with ParseHandler = js::frontend::SyntaxParseHandler; CharT = char16_t]' at js/src/shell/js.cpp:4360
    js.cpp:4357: Call(41,42, parser.SyntaxParseHandler, char16_t>(cx*,__temp_14*,options.field:0,chars*,length*,0,usedNames,0,0))
    js.cpp:4360: Call(42,43, __temp_15 := parser.checkOptions()) [[GC call]]
    js.cpp:4360: Assume(43,44, !__temp_15*, true)
    js.cpp:4361: Assign(44,45, return := 0)
    js.cpp:4361: Call(45,46, parser.~SyntaxParseHandler, char16_t>())
GC Function: _ZN2js8frontend6ParserINS0_18SyntaxParseHandlerEDsE12checkOptionsEv$bool js::frontend::Parser<ParseHandler, CharT>::checkOptions() [with ParseHandler = js::frontend::SyntaxParseHandler; CharT = char16_t]
    uint8 js::frontend::TokenStreamAnyChars::checkOptions()
    void js::frontend::TokenStreamAnyChars::reportErrorNoOffset(uint32)
    void js::ReportCompileError(JSContext*, js::ErrorMetadata*, class mozilla::UniquePtr<JSErrorNotes, JS::DeletePolicy<JSErrorNotes> >, uint32, uint32, __va_list_tag*)
    void js::CompileError::throwError(JSContext*)
    void js::ErrorToException(JSContext*, JSErrorReport*, (JSErrorFormatString*)(void*,uint32)*, void*)
    IndirectCall: callback

Comment 9

[Steve Fink [:sfink] [:s:]]

Attachment: Switch to using in-source annotations more directly

I know this is using a shotgun to kill a fly, but I needed it for other reasons too.

The issue was that even though I tagged Parser with MOZ_HAZ_ROOTED, it had no effect on the analysis. And that turned out to be because that annotation is pretty much ignored; Rooted-ness is determined entirely by hardcoded rules in annotations.js. Which is pretty lame.

This patch moves most of the rooted tyeps into a separate extraRootedPointers() list. In the future, both isRootedGCPointer and extraRootedPointers should really go away completely or at least be very minimal. I'd like to move away from the pattern of saying that any type beginning with "Rooted" or "PersistentRooted" should be considered rooted, and it looks like a very small number of MOZ_HAZ_ROOTED annotations should be enough to do that.

In the process, I uncovered a bug where GC suppression was being propagated to base classes instead of subclasses, and added a few annotations to remove the hazards that resulted from fixing it. (The reason why it had so little effect is that even though the right thing to do would be to propagate to subclasses, that was happening anyway because base classes show up as fields in subclasses, and GC suppression propagates to container structs already.)

Comment 10

[Jon Coppeard (:jonco)]

Comment on attachment 8910884 [details] [diff] [review]
Switch to using in-source annotations more directly

Review of attachment 8910884 [details] [diff] [review]:
-----------------------------------------------------------------

I wouldn't say I totally understand this, but it seems fine.

::: js/src/devtools/rootAnalysis/annotations.js
@@ +9,5 @@
>      "__conv" : true,
>      "__convf" : true,
>      "prerrortable.c:callback_newtable" : true,
>      "mozalloc_oom.cpp:void (* gAbortHandler)(size_t)" : true,
> +    "oomCallback" : true,

This callback only seems to be called under an AutoSuppressGC.  Why do we need this?

::: js/src/devtools/rootAnalysis/computeGCTypes.js
@@ +25,5 @@
>  
>  var structureParents = {}; // Map from field => list of <parent, fieldName>
>  var pointerParents = {}; // Map from field => list of <parent, fieldName>
>  var baseClasses = {}; // Map from struct name => list of base class name strings
> +var subClasses = {}; // Map from struct name => list of subclass  name strings

nit: double space in comment

@@ +317,2 @@
>  
> +print(JSON.stringify(typeInfo, null, 4));

Just use JSON, nice.

Comment 11

[Steve Fink [:sfink] [:s:]]

(In reply to Jon Coppeard (:jonco) from comment #10)
> Comment on attachment 8910884 [details] [diff] [review]
> Switch to using in-source annotations more directly
> 
> Review of attachment 8910884 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I wouldn't say I totally understand this, but it seems fine.
> 
> ::: js/src/devtools/rootAnalysis/annotations.js
> @@ +9,5 @@
> >      "__conv" : true,
> >      "__convf" : true,
> >      "prerrortable.c:callback_newtable" : true,
> >      "mozalloc_oom.cpp:void (* gAbortHandler)(size_t)" : true,
> > +    "oomCallback" : true,
> 
> This callback only seems to be called under an AutoSuppressGC.  Why do we
> need this?

Heh. Because I had a bug. (Inverted conditional.)

> @@ +317,2 @@
> >  
> > +print(JSON.stringify(typeInfo, null, 4));
> 
> Just use JSON, nice.

Nice for small stuff. Horrifically slow for big things, which is why I did all that structured cloning in the other patch. But this is small.

Comment 12

[Steve Fink [:sfink] [:s:]]

PersistentRooted does not work either. It looks like it's mainthread only, I guess because it appends to a runtime list. And Parsers can get created offthread.

Rooted didn't work because BytecodeCompiler's parser.emplace(...) is called after other Rooteds have been created. For example, we see this ordering:

 1. construct BytecodeCompiler (with an uninit Maybe<Parser> field)
 2. in compileGlobalScript(), construct a GlobalSharedContext which has a Rooted<GlobalScope::Data*> field
 3. then call compileScript(), which does emplace() on the BytecodeCompiler's Maybe<Parser>

I'm not sure of the simplest way to fix this. Make a threadsafe PersistentRooted? Switch BytecodeCompiler's Maybe<Parser> to Rooted<Maybe<Parser>>? Use Rooted<Parser*> and heap-allocate?

I'm punting for now. I'm going to make a separate bug for the analysis changes, which are independently useful.

Comment 13

[Steve Fink [:sfink] [:s:]]

Analysis patch moved to bug 1402504.