Last Comment Bug 684559 - YARR interpreter performance sucks compared to PCRE
: YARR interpreter performance sucks compared to PCRE
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: Other All
: -- normal (vote)
: mozilla9
Assigned To: Cameron Kaiser [:spectre]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 691898
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-03 21:45 PDT by Cameron Kaiser [:spectre]
Modified: 2011-12-24 01:00 PST (History)
11 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Restore PCRE for non-YARR JIT architectures (380.01 KB, patch)
2011-09-07 07:58 PDT, Cameron Kaiser [:spectre]
no flags Details | Diff | Splinter Review
Restore PCRE for non-YARR JIT architectures (v2) (377.82 KB, patch)
2011-09-08 08:05 PDT, Cameron Kaiser [:spectre]
dmandelin: review+
emorley: checkin+
Details | Diff | Splinter Review
Fix build (901 bytes, patch)
2011-09-18 04:30 PDT, Landry Breuil (:gaston)
no flags Details | Diff | Splinter Review
Remove inclusion of now removed jswtfbridge.h (863 bytes, patch)
2011-09-20 09:29 PDT, Landry Breuil (:gaston)
dmandelin: review+
emorley: checkin+
Details | Diff | Splinter Review

Description Cameron Kaiser [:spectre] 2011-09-03 21:45:52 PDT
While preparing TenFourFox 7 for release to our beta audience, during conformance testing I noticed that my Dromaeo numbers suddenly got cut in half compared to 6. The quad G5, for example, scored 110+ runs/sec in 6 but barely managed 65 in 7. Virtually all of this loss was on the regexp benchmark.

Although I understand YARR's pure interpreter performance is poorer than PCRE, this is abysmally bad. As proof, when I simply added PCRE back to jsregexp.cpp and jsregexpinlines.h, the numbers went back to normal immediately, proving the source of the regression.

While we are fortunate to have a working tracejit now on PowerPC (bug 624164), methodjit and macro assembler is still under construction and at least a couple cycles away, and keeping up with rapid release isn't making it any easier. For those tier-3 systems completely dependent on the interpreter with no JIT ability at all, this is a pretty nasty performance hit. Therefore, this would certainly help these marginal and less supported systems, and being only invoked when YARR's JIT is unavailable, would be NPOTB for all shipping tier-1 and Sparc. Ultimately in PPC land we plan to join the YARR_JIT party too, but this helps the rest.

Before I generate a pretty patch and rebase it against mozilla-central, however, I'd like to know if it would be accepted. We're shipping it in TenFourFox 7 regardless though, since we can't afford a hit like this.
Comment 1 David Mandelin [:dmandelin] 2011-09-06 11:55:20 PDT
My initial thinking is that we'd take a patch that restores PCRE only for platforms that don't use the JITs. The reasons we took out PCRE were that it [the version used in WebKit] had a model that didn't fit closely with ECMAScript regexp semantics, requiring a lot of tricky adaptations, and that it was hard to maintain and Apple wouldn't be working it anymore. But you have a good point about its current usefulness for PowerPC. By the way, I've heard that more recent versions of PCRE are generally better, and nicer for JS.

Side note: if you bring in a PowerPC assembler for the methodjit stuff, that should take care of a lot of what IonMonkey needs. David Anderson could tell you more about our plans and what kinds of things will be needed for IonMonkey.
Comment 2 Cameron Kaiser [:spectre] 2011-09-06 12:06:06 PDT
Yes, I definitely agree that this should only be for those platforms with no methodjit capability currently or otherwise. I had to hack PCRE a little anyway to make it happier with current regexes. I haven't tried working with later PCREs; this is simply the PCRE from Mozilla 6 with my own tweaks.

W/r/t PowerPC assembler support, I've finished a first draft of the JaegerTrampoline functions and I'm starting to work on MacroAssemblerPPC.h and PPCAssembler.h based on the ARM ABI, which is the closest to PPC ABI. But completing it is still a ways off, and like I say, other minority architectures will also be affected by this and will have the most to lose from such a regression. (I note that there are hooks in YARR for SH4 and MIPS but these don't appear to be in Mozilla's tree.)

I'll generate a patch for review. Would you be willing to review it, or who should I ask?
Comment 3 David Mandelin [:dmandelin] 2011-09-06 12:07:02 PDT
(In reply to Cameron Kaiser from comment #2)
> I'll generate a patch for review. Would you be willing to review it, or who
> should I ask?

I'll review it.
Comment 4 Cameron Kaiser [:spectre] 2011-09-07 07:58:08 PDT
Created attachment 558810 [details] [diff] [review]
Restore PCRE for non-YARR JIT architectures

Thanks! I took the liberty of cleaning up some of the ifdefs. This patch uses PCRE on systems where ENABLE_YARR_JIT is not enabled; the YARR interpreter is still used as the fallback in YARR if the JIT fails.
Comment 5 Cameron Kaiser [:spectre] 2011-09-07 07:59:09 PDT
(Most of it is just adding back the PCRE files; the actual changes are quite small)
Comment 6 David Mandelin [:dmandelin] 2011-09-07 12:08:05 PDT
Comment on attachment 558810 [details] [diff] [review]
Restore PCRE for non-YARR JIT architectures

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

I think js/src/yarr/jswtfbridge.h is not necessary now--I think that was only for the old Yarr port. Can you try deleting that and check that it still works? Otherwise, looks good--I don't recall exactly what the ifdefs looked like, but it does look cleaner than what I remember.
Comment 7 Cameron Kaiser [:spectre] 2011-09-07 13:29:12 PDT
It needs it for the typedefs. I can just roll them into yarr/pcre/pcre.h if that's fine with you.
Comment 8 David Mandelin [:dmandelin] 2011-09-07 13:58:05 PDT
(In reply to Cameron Kaiser from comment #7)
> It needs it for the typedefs. I can just roll them into yarr/pcre/pcre.h if
> that's fine with you.

Sounds good.
Comment 9 Cameron Kaiser [:spectre] 2011-09-08 08:05:25 PDT
Created attachment 559163 [details] [diff] [review]
Restore PCRE for non-YARR JIT architectures (v2)

Revised with needed pieces of jswtfbridge.h rolled into pcre.h.
Comment 10 Cameron Kaiser [:spectre] 2011-09-08 10:58:03 PDT
Thanks for the r+ :) Checkin requested.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2011-09-09 15:01:54 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/52550df07858
Comment 13 Justin Wood (:Callek) 2011-09-09 22:46:36 PDT
(In reply to Boris Zbarsky (:bz) from comment #11)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/52550df07858

(In reply to Phil Ringnalda (:philor) from comment #12)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/d09ca67df777

both on m-c now
Comment 14 Christian Holler (:decoder) 2011-09-11 17:35:01 PDT
Does this patch affect any platforms that we support? (PowerPC isn't on our list of supported architectures if I remember correctly).
Comment 15 Cameron Kaiser [:spectre] 2011-09-12 08:16:12 PDT
No. I wrote it so it only enables PCRE on systems that don't use the YARR JIT. All of the shipping platforms use it, as well as SPARC. On those systems the YARR interpreter is still used as the fallback. This only affects PPC and those other tier-3 systems that don't have methodjit, and none of them are supported now.
Comment 16 Landry Breuil (:gaston) 2011-09-18 04:28:49 PDT
Hm, i think there's an issue with what got commited to m-c. As of now, i can't build it on sparc64, see http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/156/steps/build/logs/stdio

It looks there's a typo, as yarr/jswtfbridge.h is included in pcre_regex.cpp.
Comment 17 Landry Breuil (:gaston) 2011-09-18 04:30:50 PDT
Created attachment 560774 [details] [diff] [review]
Fix build

Dunno if that's a correct fix but at least the build goes further with this on openbsd/sparc64.
Comment 18 Cameron Kaiser [:spectre] 2011-09-19 14:52:35 PDT
Comment on attachment 560774 [details] [diff] [review]
Fix build

Whoops, I forgot that one. (It built on mine since I still have jswtfbridge.h around for something else.) However, you shouldn't need to include wtfbridge.h either; it should all be in pcre.h. Does that not work?

I'm not a JS peer, so dmandelin should do the review.
Comment 19 Cameron Kaiser [:spectre] 2011-09-19 14:53:38 PDT
Also, does sparc64 not work with the existing Sparc JIT?
Comment 20 Landry Breuil (:gaston) 2011-09-19 15:24:29 PDT
(In reply to Cameron Kaiser from comment #19)
> Also, does sparc64 not work with the existing Sparc JIT?

No, it's broken because of alignment issues on 64 bits, see at least bug #577056. sparc64 is broken since fx 4, but i'm still trying to keep it in a 'compilable' state...
Comment 21 Landry Breuil (:gaston) 2011-09-19 15:30:12 PDT
(In reply to Cameron Kaiser from comment #18)
> Comment on attachment 560774 [details] [diff] [review]
> Fix build
> 
> Whoops, I forgot that one. (It built on mine since I still have
> jswtfbridge.h around for something else.) However, you shouldn't need to
> include wtfbridge.h either; it should all be in pcre.h. Does that not work?

Testrun with #inclusion of (js|)wtfbridge.h removed will run at http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/161, results tmrw.

> I'm not a JS peer, so dmandelin should do the review.

Will see what my build #161 says before reasking a review...
Comment 22 Cameron Kaiser [:spectre] 2011-09-19 16:19:16 PDT
Okay. Reenabling PCRE is likely to help your runtime also. When I get back to my desk and out of this meeting I'll verify this on the G5.
Comment 23 Landry Breuil (:gaston) 2011-09-20 02:49:52 PDT
Hm, removing #include "yarr/jswtfbridge.h" and not including wtfbrige.h instead is a no-go :

src/jsinfer.cpp:6041: error: 'struct JSScript' has no member named 'resetUseCount'
See http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/163/steps/build/logs/stdio

So it seems wtfbridge.h should be included..
Comment 24 Cameron Kaiser [:spectre] 2011-09-20 07:39:11 PDT
That seems fine to me pending David's review. Thanks for spotting it; that'll teach me to run builds with a dirty internal tree.
Comment 25 Landry Breuil (:gaston) 2011-09-20 09:28:13 PDT
Oh wait, i am actually mistaken. The 'new error' i'm seeing is unrelated to wtfbridge.h inclusion or not. That's a fallout of http://hg.mozilla.org/mozilla-central/rev/9ca3d16d575c for which i'm going to open a new bug (resetUseCount is defined within #ifdef JS_METHODJIT, but used outside of it).
So you were right, the inclusion of wtfbridge can probably go away. new patch.
Comment 26 Landry Breuil (:gaston) 2011-09-20 09:29:32 PDT
Created attachment 561216 [details] [diff] [review]
Remove inclusion of now removed jswtfbridge.h
Comment 27 Landry Breuil (:gaston) 2011-09-20 09:42:13 PDT
And for the record, it's https://bugzilla.mozilla.org/show_bug.cgi?id=687885
Comment 28 David Mandelin [:dmandelin] 2011-09-20 10:55:15 PDT
Comment on attachment 561216 [details] [diff] [review]
Remove inclusion of now removed jswtfbridge.h

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

My bad for missing this on review--I was the one who asked to have that file removed in the first place!
Comment 29 Cameron Kaiser [:spectre] 2011-09-20 10:57:02 PDT
Requesting check-in to fix the (limited) bustage.
Comment 30 Ed Morley [:emorley] 2011-09-21 05:42:35 PDT
Comment on attachment 561216 [details] [diff] [review]
Remove inclusion of now removed jswtfbridge.h

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7233b484b95
Comment 31 Ed Morley [:emorley] 2011-09-21 18:18:15 PDT
Comment on attachment 561216 [details] [diff] [review]
Remove inclusion of now removed jswtfbridge.h

https://hg.mozilla.org/mozilla-central/rev/c7233b484b95
Comment 32 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2011-10-12 15:11:59 PDT
sec review triage = removing flag
Comment 33 Chris Leary [:cdleary] (not checking bugmail) 2011-10-14 03:32:32 PDT
I just wanted to give a heads up that this will be #error'ing due to the lazy RegExpPrivate changes in bug 673188 -- it seems that bug 691898 is also an outstanding breakage.
Comment 34 Cameron Kaiser [:spectre] 2011-10-14 07:40:58 PDT
Yeah, I can't see an easy way that PCRE can check syntax without trying to build the regexp, unless simply trying to build it (+/- throwing away the result) is acceptable.

On the other hand the results reported are encouraging so bug 673188 may obviate the need for this bug anyway.
Comment 35 Ed Morley [:emorley] 2011-10-21 09:49:12 PDT
I believe bug 684039 may have broken this for you, see bug 684039 comment 46 for more info.
Comment 36 Cameron Kaiser [:spectre] 2011-10-21 20:04:42 PDT
It appears so. It may be better for me just to turn things back to YARR interpreter for everyone, assuming no objections.

Note You need to log in before you can comment on or make changes to this bug.