The default bug view has changed. See this FAQ.

IonMonkey: implement MRegExp as a warmup op

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cdleary, Assigned: cdleary)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 593913 [details] [diff] [review]
MRegExp without TI-based native optimizations

Starting to get my benchmark on.
Attachment #593913 - Flags: review?(jdemooij)
Whoops, left a masm.breakpoint comment in there. Will remove that before commit.
Blocks: 723640
Comment on attachment 593913 [details] [diff] [review]
MRegExp without TI-based native optimizations

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

Looks good, r=me with the comments below addressed.

::: js/src/ion/CodeGenerator.cpp
@@ +261,5 @@
> +    static const VMFunction js_CloneRegExpObjectInfo = FunctionInfo<pf>(js_CloneRegExpObject);
> +
> +    //masm.breakpoint();
> +    pushArg(ImmWord(lir->mir()->source()));
> +    pushArg(ImmWord(proto));

Use ImmGCPtr instead of ImmWord when baking in pointers to GC things, so that they can be relocated. And remove the masm.breakpoint() comment while you're here.

::: js/src/ion/IonBuilder.cpp
@@ +3411,5 @@
> +    MRegExp *ins = MRegExp::New(reobj, MRegExp::MustClone);
> +    current->add(ins);
> +    current->push(ins);
> +
> +    return true;

I don't think we want to execute this instruction twice when we bailout (see also the AliasSet comment below), so you'll need something like

if (ins->isEffectful() && !resumeAfter(ins))
    return false;

::: js/src/ion/MIR.h
@@ +2033,5 @@
> +        MustClone
> +    };
> +
> +  private:
> +    RegExpObject *source_;

I think this should be HeapPtr<RegExpObject> nowadays, see also MNewArray::type_.

@@ +2062,5 @@
> +    CloneBehavior shouldClone() const {
> +        return shouldClone_;
> +    }
> +    AliasSet getAliasSet() const {
> +        return AliasSet::None();

This means the instruction is not effectful, so for instance DCE will remove MRegExp if it has no uses and we may execute this JSOP_REGEXP twice if an instruction following it bails out. I'm not sure if that's okay.

It's probably best to use "all side-effects" for now (if shouldClone == MustClone), but we should discuss this with dvander and sstangl.

::: js/src/ion/x86/CodeGenerator-x86.h
@@ +101,5 @@
>      bool visitWriteBarrierT(LWriteBarrierT *barrier);
>      bool visitLoadElementT(LLoadElementT *load);
>      bool visitStoreElementV(LStoreElementV *store);
>      bool visitStoreElementT(LStoreElementT *store);
> +    bool visitMRegExp(LRegExp *lir);

You can remove this line.

::: js/src/jsinterp.cpp
@@ +886,5 @@
>              *equal = true;
>              return true;
>          }
> +        JS_ASSERT_IF(lref.isBoolean(), lref.isTrue() || lref.isFalse());
> +        JS_ASSERT_IF(rref.isBoolean(), rref.isTrue() || rref.isFalse());

Nit: remove these asserts?

::: js/src/jsscriptinlines.h
@@ +168,5 @@
>      JSObjectArray *arr = regexps();
>      JS_ASSERT(uint32_t(index) < arr->length);
>      JSObject *obj = arr->vector[index];
>      JS_ASSERT(obj->isRegExp());
> +    return (js::RegExpObject *) obj;

Nit: Can we replace the last two lines with obj->asRegExp()?
Attachment #593913 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij (:jandem) from comment #2)
> 
> And remove the masm.breakpoint() comment while you're here.

Ignore this line, I missed comment 1.
(In reply to Jan de Mooij (:jandem) from comment #2)
> Use ImmGCPtr instead of ImmWord when baking in pointers to GC things, so
> that they can be relocated.

Okay, I'm guessing this is future-proofing for a time when we persist code across moving GCs? It was my understanding we could generally burn in pointers under the assumption code would get thrown out on each GC.

> I don't think we want to execute this instruction twice when we bailout

It's fine if we do, we just create an additional RegExp object. We generally assume bailouts to be rare, right?

> I think this should be HeapPtr<RegExpObject> nowadays, see also
> MNewArray::type_.

Okay, more future proofing? Sounds good.

> @@ +2062,5 @@
> > +    CloneBehavior shouldClone() const {
> > +        return shouldClone_;
> > +    }
> > +    AliasSet getAliasSet() const {
> > +        return AliasSet::None();
> 
> This means the instruction is not effectful, so for instance DCE will remove
> MRegExp if it has no uses and we may execute this JSOP_REGEXP twice if an
> instruction following it bails out. I'm not sure if that's okay.

Yeah, that's ok.
 
> It's probably best to use "all side-effects" for now (if shouldClone ==
> MustClone), but we should discuss this with dvander and sstangl.

I don't understand?

> ::: js/src/jsinterp.cpp
> @@ +886,5 @@
> >              *equal = true;
> >              return true;
> >          }
> > +        JS_ASSERT_IF(lref.isBoolean(), lref.isTrue() || lref.isFalse());
> > +        JS_ASSERT_IF(rref.isBoolean(), rref.isTrue() || rref.isFalse());
> 
> Nit: remove these asserts?

How about I move them to isTrue / isFalse? Nice to detect corrupted boolean values with an assert rather than a strange assertEq failure like I got (where the type was appropriately boolean but it was neither true nor false).

> ::: js/src/jsscriptinlines.h
> @@ +168,5 @@
> >      JSObjectArray *arr = regexps();
> >      JS_ASSERT(uint32_t(index) < arr->length);
> >      JSObject *obj = arr->vector[index];
> >      JS_ASSERT(obj->isRegExp());
> > +    return (js::RegExpObject *) obj;
> 
> Nit: Can we replace the last two lines with obj->asRegExp()?

I would have liked to, but it seems to require a circular js*inlines include to avoid the "annotated inline but no inline definition is available" warning in GCC, so I just duplicated it there. :-(
(In reply to Jan de Mooij (:jandem) from comment #2)
> @@ +2062,5 @@
> > +    CloneBehavior shouldClone() const {
> > +        return shouldClone_;
> > +    }
> > +    AliasSet getAliasSet() const {
> > +        return AliasSet::None();
> 
> This means the instruction is not effectful, so for instance DCE will remove
> MRegExp if it has no uses and we may execute this JSOP_REGEXP twice if an
> instruction following it bails out. I'm not sure if that's okay.
> 
> It's probably best to use "all side-effects" for now (if shouldClone ==
> MustClone), but we should discuss this with dvander and sstangl.

LICM would be needed on MRegExp for the first loop of string-validate-input.
Hey David, would you mind weighing in on the "is a potentially GCing operation effectful" question?  I don't really buy the argument in bug 685838 comment 21, just because our LIR ops are all "atomically" interacting with the GC in a sense -- you'll never reorder a LIR op between two "halves" of a GC operation.
Potentially GC operations aren't effectful, but for operations which are expensive or allocate a lot of memory, we might not want to execute them twice. For example, a NEWARRAY with 50,000 slots can be repeated, but we probably don't want to repeat it.

I don't know how expensive JSOP_REGEXP is. If it's just allocating a JSObject, that doesnt seem too bad. If it's compiling a regular expression too, maybe not.
(In reply to David Anderson [:dvander] from comment #7)
> I don't know how expensive JSOP_REGEXP is. If it's just allocating a
> JSObject, that doesnt seem too bad. If it's compiling a regular expression
> too, maybe not.

The latter! Agreed on the many-slot concern, but this one should be a drop in the bucket. Thanks for the feedback!
(In reply to Chris Leary [:cdleary] from comment #8)
> (In reply to David Anderson [:dvander] from comment #7)
> > I don't know how expensive JSOP_REGEXP is. If it's just allocating a
> > JSObject, that doesnt seem too bad. If it's compiling a regular expression
> > too, maybe not.

Er, the middle-r. No compilation.
https://hg.mozilla.org/projects/ionmonkey/rev/f6b54f8e87b2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Looks like this may have regressed testMatchStringObject.js and/or testLet.js -- not sure why I didn't see those locally. I'll back out and investigate.

https://hg.mozilla.org/projects/ionmonkey/rev/f7f49506bdc9 (backout)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blah, args were being pushed first-to-last instead of last-to-first.

https://hg.mozilla.org/projects/ionmonkey/rev/902afe4fb2fa
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.