Closed Bug 745742 Opened 9 years ago Closed 8 years ago

Get rooting analysis to pass jit-tests


(Core :: JavaScript Engine, defect)

Not set





(Reporter: bhackett1024, Assigned: bhackett1024)



(Whiteboard: [Leave open after merge])


(2 files, 1 obsolete file)

Bug 714647 got this most of the way there (hopefully!).
Attached patch WIP (obsolete) — Splinter Review
Passes about 80% of jit-tests in the interpreter.
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.
Assignee: general → bhackett1024
Attachment #616845 - Attachment is obsolete: true
Attachment #617561 - Flags: review?(wmccloskey)
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 @@
>  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?
Attachment #617561 - Flags: review?(wmccloskey) → review+
Attached patch rebasedSplinter Review
Brian, I needed this to debug the generational gc, so I went and rebased.  Hope it saves you some time.
Attachment #619232 - Flags: review+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Looks like this broke windebug SM builds: <>
Resolution: FIXED → ---
Blocks: 751567
I landed fixes for the various build warnings this introduced -- basically from passing in |this| when constructing member variables, using the old |thisDuringConstruction()| trick:

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:

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...
Whiteboard: [Leave open after merge]
(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?
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.
Depends on: ExactRooting
Blocks: ExactRooting
No longer depends on: ExactRooting
Depends on: 830570
Depends on: 830581
Depends on: 830583
Depends on: 830586
Depends on: 830589
Depends on: 830590
Depends on: 830332
Depends on: 833340
Duplicate of this bug: 831580
The rooting analysis is green on TBPL now.
Closed: 9 years ago8 years ago
Resolution: --- → FIXED
Nevermind, this bug still has a long-term nit for windows builds that we need to fix.
Resolution: FIXED → ---
Depends on: 821123
Depends on: 884934
No longer blocks: GenerationalGC
We're switching to a different mechanism for dynamic rooting analysis.
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.