Reduce cost of stack rooting during name operations

RESOLVED FIXED in mozilla21

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bhackett, Unassigned)

Tracking

Other Branch
mozilla21
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 704542 [details] [diff] [review]
patch

Name operations are a large source of root construction, both at the top level and within the various lookup operations performed underneath.  The attached patch removes almost all of these.  It also makes some improvements to GC thing allocation so that non-GC'ing allocation can allocate new arenas without triggering a GC (thus, non-GC'ing allocation will almost always succeed).

This eliminates about 16% of the original 3.7 million roots on SS, bringing the number of created roots to about 61% of the original count.
Attachment #704542 - Flags: review?(terrence)
Just wondering why you don't used Unrooted* types?
(Reporter)

Comment 2

5 years ago
Well, the patch adds raw outparams too, which there isn't an Unrooted substitute for, and I'd rather not mix styles.  More than that though, I don't think the Unrooted type adds anything that the static and dynamic rooting analyses do not already pick up.  Eventually I think we'll want to rm the Unrooted and Raw types, but that's not a high priority and things are in flux right now.
(In reply to :Ms2ger from comment #1)
> Just wondering why you don't used Unrooted* types?

What Brian said. The static analysis makes our Unrooted dynamic analysis more-or-less completely obsolete: e.g., more trouble that it is worth to maintain. Once the rooting analysis is on TBPL and stuck we are going to rip out all Unrooted/AssertCanGC/AutoAssertNoGC bits and settle on consistent types.
Comment on attachment 704542 [details] [diff] [review]
patch

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

This is a thing of beauty and I wish I'd thought to do it this way when pushing Handles through jsscope. I do have a couple questions that I need answers for, however, before I can finish reviewing it.

::: js/src/jsgc.cpp
@@ +1462,5 @@
>      JS_ASSERT(!rt->isHeapBusy());
>  
> +    bool runGC = rt->gcIncrementalState != NO_INCREMENTAL
> +        && comp->gcBytes > comp->gcTriggerBytes
> +        && allowGC;

Style nit: && at the end of lines and align left hand side with rt.

::: js/src/jsobj.cpp
@@ +3261,5 @@
>      return true;
>  }
>  
> +template <> JS_ALWAYS_INLINE JSBool
> +CallResolveOp<DONT_ALLOW_GC>(JSContext *cx, JSObject *obj, jsid id, unsigned flags,

This strategy seems like a fairly excessive way to do |if (!allowGC) return NULL;|. Could we not just make this check directly in lookupPropertyWithFlagsInline and only have variants of CallResolveOp and CallLookupGeneric for ALLOW_GC? I believe, that would also keep us from needing FakeRooted and FakeMutableHandle. Or am I missing something subtle here?

@@ +3405,5 @@
> +                   JSObject **objp, JSObject **pobjp, Shape **propp)
> +{
> +    JS_ASSERT(!*objp && !*pobjp && !*propp);
> +
> +    AutoAssertNoGC nogc;

Move above the JS_ASSERT and remove the whitespace between them.

@@ +3407,5 @@
> +    JS_ASSERT(!*objp && !*pobjp && !*propp);
> +
> +    AutoAssertNoGC nogc;
> +
> +    RootedId id(cx, NameToId(name));

It looks like this |id| is unused, and an excess root.

::: js/src/jsstr.cpp
@@ +2009,5 @@
>          JSAtom *atom;
>          if (str->isAtom()) {
>              atom = &str->asAtom();
>          } else {
>              atom = AtomizeString(cx, str);

ReplaceData is allocated on the stack in str_replace, so it seems like this AtomizeString call would kill rdata.elembase. What does the static analyzer know that I don't?

::: js/src/vm/String.h
@@ +660,1 @@
>      static inline JSInlineString *new_(JSContext *cx);

Thanks for cleaning this up while you were here.
(Reporter)

Comment 5

5 years ago
(In reply to Terrence Cole [:terrence] from comment #4)

> This strategy seems like a fairly excessive way to do |if (!allowGC) return
> NULL;|. Could we not just make this check directly in
> lookupPropertyWithFlagsInline and only have variants of CallResolveOp and
> CallLookupGeneric for ALLOW_GC? I believe, that would also keep us from
> needing FakeRooted and FakeMutableHandle. Or am I missing something subtle
> here?

The problem is that even if the CallResolveOp call is unreachable in the !allowGC case, it will still fail to type check because it is being passed bare pointers instead of handles.  I would prefer it if the code could be made entirely conditional on ALLOW_GC, but I don't think that is possible to do in C++ without this kind of function specialization.

FWIW, the main uses of FakeRooted and FakeMutableHandle are in MaybeRooted<DONT_ALLOW_GC>.

> It looks like this |id| is unused, and an excess root.

Oops, good catch.

> ReplaceData is allocated on the stack in str_replace, so it seems like this
> AtomizeString call would kill rdata.elembase. What does the static analyzer
> know that I don't?

rdata.elembase, and the other fields of ReplaceData, are still Rooted.  These roots account for about 10k hits (see bug 831886) which might be nice to improve but I think doing so would be pretty painful.  FindReplaceLength accounts for 90k hits as it is called much more often than str_replace.
(In reply to Brian Hackett (:bhackett) from comment #5)
> (In reply to Terrence Cole [:terrence] from comment #4)
> 
> > This strategy seems like a fairly excessive way to do |if (!allowGC) return
> > NULL;|. Could we not just make this check directly in
> > lookupPropertyWithFlagsInline and only have variants of CallResolveOp and
> > CallLookupGeneric for ALLOW_GC? I believe, that would also keep us from
> > needing FakeRooted and FakeMutableHandle. Or am I missing something subtle
> > here?
> 
> The problem is that even if the CallResolveOp call is unreachable in the
> !allowGC case, it will still fail to type check because it is being passed
> bare pointers instead of handles.  I would prefer it if the code could be
> made entirely conditional on ALLOW_GC, but I don't think that is possible to
> do in C++ without this kind of function specialization.
> 
> FWIW, the main uses of FakeRooted and FakeMutableHandle are in
> MaybeRooted<DONT_ALLOW_GC>.

Ah, right, I had forgotten about that. Hmm, since the code isn't reached we only have to reify the types: the unrooted variants aren't needed for performance and if we generate extra rooting code it should be discarded by the optimizer. I believe something like the following should produce basically the same assembly with less code:

// Definition:
template <typename T> struct RootIfUnrooted {};
template <typename T> struct RootIfUnrooted<MaybeRooted<T, ALLOW_GC>::HandleType> {
    Handle<T> handle;
    typedef RootIfUnrooted<MaybeRooted<T, ALLOW_GC>::HandleType> OwnType;
    OwnType(JSContext *cx, Handle<T> init) : handle(init) {}
    Handle<T> get() { return handle; }
};
template <typename T> struct RootIfUnrooted<MaybeRooted<T, DONT_ALLOW_GC>::HandleType> {
    Rooted<T> root;
    typedef RootIfUnrooted<MaybeRooted<T, DONT_ALLOW_GC>::HandleType> OwnType;
    OwnType(JSContext *cx, T init) : root(cx, init) {}
    Handle<T> get() { return Handle<T>(root); }
};

// Usage:
MaybeRooted<JSObject*, allowGC>::HandleType arg;
RootIfUnrooted<MaybeRooted<JSObject*, allowGC>::HandleType> argRoot(cx, arg);
CallResolveOp(arg.get(), ...);

The only potential downside I see is that the compiler might actually generate the unreachable code. I think, however, that if we add the explicit allowGC check at the branch head, it shouldn't. We can double-down here too and add a JS_UNLIKELY to ensure that the code at least gets put out-of-line if it does get generated. 

> > ReplaceData is allocated on the stack in str_replace, so it seems like this
> > AtomizeString call would kill rdata.elembase. What does the static analyzer
> > know that I don't?
> 
> rdata.elembase, and the other fields of ReplaceData, are still Rooted. 
> These roots account for about 10k hits (see bug 831886) which might be nice
> to improve but I think doing so would be pretty painful.  FindReplaceLength
> accounts for 90k hits as it is called much more often than str_replace.

Ah, good to know! From what I recall, the string replacement paths are in serious need of modernization.
(Reporter)

Comment 7

5 years ago
Created attachment 705331 [details] [diff] [review]
updated

Nice, that sort of strategy does look better.  Updated patch which works a little differently: adds toHandle and toMutableHandle methods to MaybeRooted that will null crash if used with types that have not been rooted.
Attachment #704542 - Attachment is obsolete: true
Attachment #704542 - Flags: review?(terrence)
Attachment #705331 - Flags: review?(terrence)
Comment on attachment 705331 [details] [diff] [review]
updated

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

Yup, that's much nicer. I am a bit disappointed that it doesn't also remove the need for FakeRooted, but I can live with that. Is there more to FakeRooted than providing the JSContext constructor? If not, it would be much less code to make FakeRooted a wrapper like the RootIfUnrooted class from comment 6.
Attachment #705331 - Flags: review?(terrence) → review+
(Reporter)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c207bdc9a324

The FakeRooted class is adding interface methods that appear in Rooted or MutableHandle but do not appear in the bare underlying type.  This allows code to use the fake values in the same way as normal Roots and MutableHandles (e.g. for outp.set(...) or &root accesses), without cluttering up the code with additional variables.
https://hg.mozilla.org/mozilla-central/rev/c207bdc9a324
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.