Closed Bug 951439 Opened 6 years ago Closed 6 years ago
Monkey: use Call VM for Reg Exp .exec()
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.
Backed out for various bustages. Please run this through Try before pushing again. https://hg.mozilla.org/integration/mozilla-inbound/rev/a6f4adade9df https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=11751c0efe27
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.
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?)
You need to log in before you can comment on or make changes to this bug.