Closed Bug 951439 Opened 6 years ago Closed 6 years ago

IonMonkey: use CallVM for RegExp.exec()

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: sstangl, Assigned: sstangl)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Attached patch patchSplinter Review
Avoiding LCallNative in favor of an LRegExpExec appears to consistently move octane-regexp from ~2700 to ~2740 on x64. That's a speedup of 1-2%! Yowza!
Attachment #8349082 - Flags: review?(hv1989)
Comment on attachment 8349082 [details] [diff] [review]
patch

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

So I had this patch too. I didn't bother to upload, since for me the numbers are in the noise. So I was even thinking there is no real difference in generated code...
Can you check the generated code w/wo this patch and see we indeed have better codegen?
If it is indeed better r=me
Attachment #8349082 - Flags: review?(hv1989) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/11751c0efe27

I didn't check the generated code, but it's a noticeable improvement on both of my machines. It would indeed be interesting to find the inefficiencies on the main call path. I'll check into that soon.
I accidentally wrote "1" instead of "BOX_PIECES", so local testing on one arch wasn't useful. So as to not be obstinate, I pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=d3bc997cf1c1
Comment on attachment 8349082 [details] [diff] [review]
patch

>@@ -1064,6 +1066,34 @@ IonBuilder::inlineStrCharAt(CallInfo &callInfo)
[...]
>+    callInfo.unwrapArgs();

This didn't compile (and is currently holding inbound closed) because this 'unwrapArgs' method does not exist.
https://hg.mozilla.org/mozilla-central/rev/72af8e9c325c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(In reply to Sean Stangl [:sstangl] from comment #0)
> Avoiding LCallNative in favor of an LRegExpExec appears to consistently move
> octane-regexp from ~2700 to ~2740 on x64. That's a speedup of 1-2%! Yowza!

FYI this didn't really increase octane2.0-regexp scores. If something it would decrease performance of octane2.0-regexp. If the exec() would have been replaced, we would have a regression of 21%, since we lost type information.
?Luckily? the function RegExp sees an undefined as str to re.exec(str) (by accident, I hope). So we can't replace the exec() call to MRegExpExec. (Who thought this was a good idea -.-)

1) So we need the pushTypeBarrier to not regress.
2) Can you find out what we do when re.exec(str) the str isn't string? In my patch I just let us bailout (StringPolicy on MRegExpReplace). Now we have been very carefully to not add new cases of constant bailouts. So this isn't the right approach...

@sstangl: can you look into this? (Maybe create a follow-up bug?)
Flags: needinfo?(sstangl)
Follow-up bug to fix problems mentioned in comment 10: bug 964229
Flags: needinfo?(sstangl)
You need to log in before you can comment on or make changes to this bug.