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.

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.

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?

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

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.

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

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.

It needs it for the typedefs. I can just roll them into yarr/pcre/pcre.h if that's fine with you.

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

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

Thanks for the r+ :) Checkin requested.

(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

Does this patch affect any platforms that we support? (PowerPC isn't on our list of supported architectures if I remember correctly).

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.

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.

Dunno if that's a correct fix but at least the build goes further with this on openbsd/sparc64.

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.

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

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

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

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.

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

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.

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.

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

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!

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

Comment on attachment 561216 [details] [diff] [review] Remove inclusion of now removed jswtfbridge.h https://hg.mozilla.org/integration/mozilla-inbound/rev/c7233b484b95

Comment on attachment 561216 [details] [diff] [review] Remove inclusion of now removed jswtfbridge.h https://hg.mozilla.org/mozilla-central/rev/c7233b484b95

sec review triage = removing flag

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.

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.

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

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