The default bug view has changed. See this FAQ.

Make JSOP_REGEXP fast

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bhackett, Unassigned)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 565398 [details] [diff] [review]
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
Attachment #565398 - Flags: review?(cdleary)
Blocks: 691314
No longer blocks: 619314
(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?
With TI we guard against this statically.  If RegExp.prototype.test changes then it will trigger deoptimization.
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?
Attachment #565398 - Flags: review?(cdleary) → review+
> ::: 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).
(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.
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
https://hg.mozilla.org/mozilla-central/rev/dfd8ef9e5049
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Depends on: 695896
Depends on: 728021
You need to log in before you can comment on or make changes to this bug.