745742 - Get rooting analysis to pass jit-tests

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Bug 714647 got this most of the way there (hopefully!).

Comment 1

[Brian Hackett [Laid off!]]

Attachment: WIP

Passes about 80% of jit-tests in the interpreter.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch (67ca169a52d2)

Passes all jit-tests in the interpreter, modulo some timeouts.  There are a couple caveats:

- The rooting analysis is disabled if e4x or Reflect.parse is used.  Both of these would require a lot of work and changes to poorly tested/exercised code which I'm not interested in undertaking (started on jsxml.cpp and eventually gave up).  This can be addressed either by removing/disabling these in the browser, or (more likely) I think wrapping this functionality in a scope-based class which enables the conservative scanner (and disables moving GC while inside it --- not sure how much of a pain this would be for GGC).  In any case the patch just punts on this for now.

- Various SkipRoots added for places where internal cursors into GC things are used (strings, arrays, typed arrays).  Again, the patch is punting on these because fixing these requires some deeper changes to the code, and this patch is just adding rooters without fiddling with underlying functionality.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 617561 [details] [diff] [review]
patch (67ca169a52d2)

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

Thanks, Brian!

::: js/src/jsapi.cpp
@@ +3651,5 @@
>              getter = JS_DATA_TO_FUNC_PTR(PropertyOp, getobj);
>              attrs |= JSPROP_GETTER;
>          }
>          if (setter) {
> +            RootObject getRoot(cx, (JSObject **) &getter);

If we root getter here, shouldn't we root setter above?

::: js/src/jsapi.h
@@ +2290,5 @@
>  JS_ALWAYS_INLINE bool
>  ToNumber(JSContext *cx, const Value &v, double *out)
>  {
>      AssertArgumentsAreSane(cx, v);
> +

Extra ' ' character here.

::: js/src/jscntxt.h
@@ +159,5 @@
>          uintptr_t       words[JS_HOWMANY(sizeof(jmp_buf), sizeof(uintptr_t))];
>      } registerSnapshot;
>  
>      ConservativeGCData()
> +    {

Usually the brace goes on the same line as the function name in these sorts of definitions.

::: js/src/jsgc.cpp
@@ +3938,5 @@
>      rt->gcDeterministicOnly = enabled;
>  #endif
>  }
>  
> +} } /* namespace js::gc */

Could you put this on two lines?

::: js/src/jstypedarray.cpp
@@ +464,1 @@
>                     PropertyOp getter, StrictPropertyOp setter, unsigned attrs)

Fix the indent here?

::: js/src/vm/RegExpStatics.h
@@ +255,4 @@
>       : original(original),
> +       buffer(RegExpStatics::InitBuffer()),
> +       matchPairsInputRoot(cx, (JSLinearString**) &buffer.matchPairsInput),
> +       pendingInputRoot(cx, (JSString**) &buffer.pendingInput)

Could you create one of those inner class rooter things for RegExpStatics like you did in a few other places?

Comment 4

[Terrence Cole [:terrence]]

Attachment: rebased

Brian, I needed this to debug the generational gc, so I went and rebased.  Hope it saves you some time.

Comment 5

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/07a4d4b0260c

Comment 6

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/07a4d4b0260c

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Looks like this broke windebug SM builds: <https://tbpl.mozilla.org/?tree=Mozilla-Inbound&noignore=1&jobname=spidermonkey&rev=07a4d4b0260c>

Comment 8

[Jeff Walden [:Waldo]]

I landed fixes for the various build warnings this introduced -- basically from passing in |this| when constructing member variables, using the old |thisDuringConstruction()| trick:

https://hg.mozilla.org/integration/mozilla-inbound/rev/489e1b75048e

Also I landed a fix for bug 751567, where MSVC is going insane complaining about a mismatched brace in code that's clearly not brace-mismatching:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4608e8e96c87

I put the minimal investigations I made in a comment in the revision.  The summary is that something about an opening brace followed by constructing an instance of a class that uses the guard-object macros triggers the issue.  An explicit scope triggers it, as does the same code in a completely separate method.  I didn't see a hackaround that would have equivalent functionality.

Given that I have no idea what's up here, and various attempts at a hackaround all failed, I gave up and just put an #ifndef _MSC_VER / #endif around the problematic code.  It's not the right long-term fix (I hope), but it gets us compiling again, at least.

Leaving open for that super-small long-term fix, whatever it might end up being...

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/489e1b75048e
https://hg.mozilla.org/mozilla-central/rev/4608e8e96c87

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to Justin Wood (:Callek) from bug 751567 comment #1)
> My initial theory is that this is failing with MSVC8 and would succeed with MSVC10

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #8)
> I [...] just put an #ifndef _MSC_VER / #endif around the problematic code.

Would something like "#if !defined(_MSC_VER) || _MSC_VER > 1400" be (more) appropriate?

Comment 11

[Jeff Walden [:Waldo]]

Perhaps, but I didn't see this bug or that bug before adding the ifdefs, so I didn't realize this went away in newer versions of MSVC.  Also I'm assuming there's a better fix that will do the root-skipping and maybe-checking even in MSVC, so it's not important how pretty whatever hackaround's there now is, because it'll get replaced with a real fix soon.

Comment 13

[Terrence Cole [:terrence]]

The rooting analysis is green on TBPL now.

Comment 14

[Terrence Cole [:terrence]]

Nevermind, this bug still has a long-term nit for windows builds that we need to fix.

Comment 15

[Terrence Cole [:terrence]]

We're switching to a different mechanism for dynamic rooting analysis.