Reduce cost of stack rooting during add operations

RESOLVED FIXED in mozilla21

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bhackett, Unassigned)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Created attachment 703617 [details] [diff] [review]
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.
Attachment #703617 - Flags: review?(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>
Attachment #703617 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/1591384ce3ad
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.