692657 - Make JSOP_REGEXP fast

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch against 15bfad783467

For regexp literals we always take a stub call to clone the object.  Instead, cloning should be inlined, and in places we can prove the regexp will never be directly accessed by a script then the cloning should be avoided altogether.  e.g. nothing can observe side effects on the regexp objects created in the fragment below.

  for (...) {
    if (/whatever/.test(str))
      ...
  }

This helps a lot with some Peacekeeper tests and, I'm sure, common real-web patterns (I've seen the above pattern in jQuery).  Times below for Peacekeeper stringDetectBrowser x 100000 (which is basically the above pattern, repeated for a dozen or so regexps), also with the bug 683804 fix.  Bug 683804 seems to be the reason for the slowdown in current JM+TI vs. pre-TI m-c shells.

js -m -n (old): 521
js -m -n (new): 234
js -m -j -p: 433
d8: 231

Comment 1

[Jeff Walden [:Waldo]]

(In reply to Brian Hackett from comment #0)
> avoided altogether.  e.g. nothing can observe side effects on the regexp
> objects created in the fragment below.
> 
>   for (...) {
>     if (/whatever/.test(str))

...assuming a shape guard or something on RegExp.prototype to guard against the "test" property not being the original function, right?

Comment 2

[Brian Hackett [Laid off!]]

With TI we guard against this statically.  If RegExp.prototype.test changes then it will trigger deoptimization.

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 565398 [details] [diff] [review]
patch against 15bfad783467

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

Good idea! r=me with these addressed.

::: js/src/jsinfer.h
@@ +324,5 @@
>  
>      /* Outer function which has been marked reentrant. */
> +    OBJECT_FLAG_REENTRANT_FUNCTION    = 0x00800000,
> +
> +    /* For a global object, whether flags were set on RegExp. */

Could we s/RegExp/the |RegExpStatics|/ here?

::: js/src/jsregexp.h
@@ +155,5 @@
>       * If not, place undefined in |*out|.
>       */
>      bool makeMatch(JSContext *cx, size_t checkValidIndex, size_t pairNum, Value *out) const;
>  
> +    /* Flags which can be set on RegExp. */

Ditto.

::: js/src/methodjit/BaseAssembler.h
@@ +1308,5 @@
>  
> +        /*
> +         * Fixed slots of non-array objects are required to be initialized;
> +         * Use the values currently in the template object.
> +         */

Is this supposed to be part of this patch? If so, I don't get it. Could you explain?

::: js/src/methodjit/Compiler.cpp
@@ +6528,5 @@
> +    JS_ASSERT((origFlags & staticsFlags) == staticsFlags);
> +
> +    /*
> +     * Don't clone regular expressions if they will be passed to a RegExp or
> +     * String native which will not leak the regexp.

Nit: can we make this a positive-logic statement? "Only clone RegExps when they may escape. Passing as an argument to a RegExp or string native cannot cause the RegExp to escape."

@@ +6530,5 @@
> +    /*
> +     * Don't clone regular expressions if they will be passed to a RegExp or
> +     * String native which will not leak the regexp.
> +     */
> +    analyze::SSAUseChain *uses = analysis->useChain(analyze::SSAValue::PushedValue(PC - script->code, 0));

Nit: overlong line.

@@ +6570,5 @@
> +    OOL_STUBCALL(stubs::RegExp, REJOIN_FALLTHROUGH);
> +
> +    /* Bump the refcount on the wrapped RegExp. */
> +    size_t *refcount = RegExp::extractFrom(regex)->addressOfRefCount();
> +    masm.add32(Imm32(1), AbsoluteAddress(refcount));

This doesn't quite work on 64b. If we overflow the 32b space the low bits will wrap around in the JIT while the high bits may be set. Can we #ifdef on 64b or add a addSize to masm?

::: js/src/methodjit/StubCalls.h
@@ +151,5 @@
>  void JS_FASTCALL SetConst(VMFrame &f, JSAtom *atom);
>  template<JSBool strict> void JS_FASTCALL DefFun(VMFrame &f, JSFunction *fun);
>  JSObject * JS_FASTCALL DefLocalFun(VMFrame &f, JSFunction *fun);
>  JSObject * JS_FASTCALL DefLocalFun_FC(VMFrame &f, JSFunction *fun);
> +void JS_FASTCALL RegExp(VMFrame &f, JSObject *regex);

We consistently use regexp instead of regex in the rest of the engine -- if you catch those could you change them?

Comment 4

[Brian Hackett [Laid off!]]

> ::: js/src/methodjit/BaseAssembler.h
> @@ +1308,5 @@
> >  
> > +        /*
> > +         * Fixed slots of non-array objects are required to be initialized;
> > +         * Use the values currently in the template object.
> > +         */
> 
> Is this supposed to be part of this patch? If so, I don't get it. Could you
> explain?

This is fixing a problem with getNewObject that was not exposed by the function's previous uses.  It used to just fill out the slots of native objects with undefined values, but when cloning a RegExp object the result needs particular bools and ints in its reserved slots (set according to the template).

> @@ +6570,5 @@
> > +    OOL_STUBCALL(stubs::RegExp, REJOIN_FALLTHROUGH);
> > +
> > +    /* Bump the refcount on the wrapped RegExp. */
> > +    size_t *refcount = RegExp::extractFrom(regex)->addressOfRefCount();
> > +    masm.add32(Imm32(1), AbsoluteAddress(refcount));
> 
> This doesn't quite work on 64b. If we overflow the 32b space the low bits
> will wrap around in the JIT while the high bits may be set. Can we #ifdef on
> 64b or add a addSize to masm?

Maybe I'm mis-grepping but it seems the only things that holds references on RegExps are RegExp objects and ExecuteRegExp stack frames.  Each RegExp object holds a single reference, so we'd need to allocate 4 billion of them, which is almost 600GB of data on x64 (~150 times the browser's maximum GC heap size), in order to overflow the refcount.  I think the refcount should be a uint32 instead (will save memory).

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to Brian Hackett from comment #4)
> I think the refcount should
> be a uint32 instead (will save memory).

Nice reasoning -- LMK if it's annoying to shoehorn those uint32 changes into this patch, I can get it in another regexp patch I'm doing instead.

Comment 6

[Brian Hackett [Laid off!]]

I didn't make the uint32 change to the refcount --- to see the benefit (such as there is) on x64 the structure needs to be rearranged some so the field can sit next to other non-word-sized fields.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfd8ef9e5049

Comment 7

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/dfd8ef9e5049