832042 - Reduce cost of stack rooting during add operations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Addition operations are the single largest source of root construction right now on SS.  The attached patch eliminates the vast majority of these roots, reducing the total number of roots constructed on SS by 23%, and without adding new overhead elsewhere.

Comment 1

[Terrence Cole [:terrence]]

Comment on attachment 703617 [details] [diff] [review]
patch

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

This is pretty awesome, there's just one thing I'd like changed.

::: js/src/gc/RootMarking.cpp
@@ +42,5 @@
>  typedef RootedValueMap::Enum RootEnum;
>  
>  #ifdef JSGC_USE_EXACT_ROOTING
>  static inline void
> +MarkExactStackRooter(JSTracer *trc, Rooted<void*> *rooter, ThingRootKind kind)

Thanks for cleaning this up.

::: js/src/jsgcinlines.h
@@ +572,5 @@
> +inline JSString *
> +js_TryNewGCString(JSContext *cx)
> +{
> +    return js::gc::TryNewGCThing<JSString>(cx, js::gc::FINALIZE_STRING, sizeof(JSString));
> +}

I added the same code this morning to remove the SkipRoot in NewShortString.

::: js/src/jsinterpinlines.h
@@ +522,5 @@
>          enabler.enable();
>  }
>  
>  static JS_ALWAYS_INLINE bool
> +AddOperation(JSContext *cx, HandleScript script, jsbytecode *pc, 

Remove the trailing space.

::: js/src/jsstr.h
@@ +40,5 @@
>   */
>  class RopeBuilder;
>  
> +extern JSString *
> +TryConcatStrings(JSContext *cx, JSString *s1, JSString *s2);

Uber-nit: I think TryConcatStrings is probably not terribly useful on its own: what we use in practice is actually Try + Retry. I'd ask you to morph Try into this except that I cannot come up with a good name for the thing. None of these seem quite right:

ConcatStringsAndMaybeClobberStackRoots
ConcatStringsBimodal
ConcatStringsMaybeSlow
ConcatStringsUsuallyFast

If you can think of a good name, then go for it, otherwise it's fine as is.

::: js/src/vm/String-inl.h
@@ +131,5 @@
>      js::StringWriteBarrierPost(compartment(), &d.u1.left);
>      js::StringWriteBarrierPost(compartment(), &d.s.u2.right);
>  }
>  
> +template <bool Rooted>

See comment on ConcatStringsMaybeRooted.

::: js/src/vm/String.cpp
@@ +295,5 @@
>      return flattenInternal<NoBarrier>(maybecx);
>  #endif
>  }
>  
> +template <bool Rooted>

Calling this template parameter Rooted is unnecessarily confusing. I think it would actually make more sense to make this parameter reflect the external effects of the call: e.g. does it GC, invalidating unrooted stack pointers, or not. How about something like:

enum MaybeGC {
    NoGC,
    AllowGC
};
template <MaybeGC maybeGC>

Comment 2

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1591384ce3ad

Comment 3

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

https://hg.mozilla.org/mozilla-central/rev/1591384ce3ad