Get rooting analysis to pass jit-tests

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

unspecified
mozilla15
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Leave open after merge])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Bug 714647 got this most of the way there (hopefully!).
(Assignee)

Comment 1

6 years ago
Created attachment 616845 [details] [diff] [review]
WIP

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

Comment 2

6 years ago
Created attachment 617561 [details] [diff] [review]
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.
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 @@
>  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?
Attachment #617561 - Flags: review?(wmccloskey) → review+
Created attachment 619232 [details] [diff] [review]
rebased

Brian, I needed this to debug the generational gc, so I went and rebased.  Hope it saves you some time.
Attachment #619232 - Flags: review+
(Assignee)

Comment 5

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

Comment 6

6 years ago
https://hg.mozilla.org/mozilla-central/rev/07a4d4b0260c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Looks like this broke windebug SM builds: <https://tbpl.mozilla.org/?tree=Mozilla-Inbound&noignore=1&jobname=spidermonkey&rev=07a4d4b0260c>
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

6 years ago
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:

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...
Whiteboard: [Leave open after merge]

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/489e1b75048e
https://hg.mozilla.org/mozilla-central/rev/4608e8e96c87
(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: 753203
Blocks: 753203
No longer depends on: 753203
Depends on: 830570
Depends on: 830581
Depends on: 830583
Depends on: 830586
Depends on: 830589
Depends on: 830590

Updated

5 years ago
Depends on: 830332

Updated

5 years ago
Depends on: 833340
Duplicate of this bug: 831580
The rooting analysis is green on TBPL now.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Nevermind, this bug still has a long-term nit for windows builds that we need to fix.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 821123
Depends on: 884934
No longer blocks: 619558
We're switching to a different mechanism for dynamic rooting analysis.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.