684559 - YARR interpreter performance sucks compared to PCRE

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Cameron Kaiser [:spectre]]

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]]

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]]

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]]

(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]]

Attachment: 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]]

(Most of it is just adding back the PCRE files; the actual changes are quite small)

Comment 6

[David Mandelin [:dmandelin]]

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]]

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]]

(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]]

Attachment: Restore PCRE for non-YARR JIT architectures (v2)

Revised with needed pieces of jswtfbridge.h rolled into pcre.h.

Comment 10

[Cameron Kaiser [:spectre]]

Thanks for the r+ :) Checkin requested.

Comment 11

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/52550df07858

Comment 12

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d09ca67df777

Comment 13

[Justin Wood (:Callek)]

(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)]

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]]

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)]

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)]

Attachment: 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]]

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]]

Also, does sparc64 not work with the existing Sparc JIT?

Comment 20

[Landry Breuil (:gaston)]

(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)]

(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]]

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)]

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]]

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)]

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)]

Attachment: Remove inclusion of now removed jswtfbridge.h

Comment 27

[Landry Breuil (:gaston)]

And for the record, it's https://bugzilla.mozilla.org/show_bug.cgi?id=687885

Comment 28

[David Mandelin [:dmandelin]]

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]]

Requesting check-in to fix the (limited) bustage.

Comment 30

[Ed Morley [:emorley]]

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]]

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]]]

sec review triage = removing flag

Comment 33

[Chris Leary [:cdleary] (not checking bugmail)]

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]]

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]]

I believe bug 684039 may have broken this for you, see bug 684039 comment 46 for more info.

Comment 36

[Cameron Kaiser [:spectre]]

It appears so. It may be better for me just to turn things back to YARR interpreter for everyone, assuming no objections.