Closed
Bug 684559
Opened 13 years ago
Closed 13 years ago
YARR interpreter performance sucks compared to PCRE
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: spectre, Assigned: spectre)
References
Details
Attachments
(2 files, 2 obsolete files)
377.82 KB,
patch
|
dmandelin
:
review+
emorley
:
checkin+
|
Details | Diff | Splinter Review |
863 bytes,
patch
|
dmandelin
:
review+
emorley
:
checkin+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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.
Assignee | ||
Comment 2•13 years ago
|
||
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•13 years ago
|
||
(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.
Assignee | ||
Comment 4•13 years ago
|
||
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.
Assignee: general → spectre
Attachment #558810 -
Flags: review?(dmandelin)
Assignee | ||
Comment 5•13 years ago
|
||
(Most of it is just adding back the PCRE files; the actual changes are quite small)
Comment 6•13 years ago
|
||
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.
Attachment #558810 -
Flags: review?(dmandelin)
Assignee | ||
Comment 7•13 years ago
|
||
It needs it for the typedefs. I can just roll them into yarr/pcre/pcre.h if that's fine with you.
Comment 8•13 years ago
|
||
(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.
Assignee | ||
Comment 9•13 years ago
|
||
Revised with needed pieces of jswtfbridge.h rolled into pcre.h.
Attachment #558810 -
Attachment is obsolete: true
Attachment #559163 -
Flags: review?(dmandelin)
Updated•13 years ago
|
Attachment #559163 -
Flags: review?(dmandelin) → review+
![]() |
||
Comment 11•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/52550df07858
Flags: in-testsuite-
Keywords: checkin-needed
Comment 13•13 years ago
|
||
(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
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Target Milestone: --- → mozilla9
Comment 14•13 years ago
|
||
Does this patch affect any platforms that we support? (PowerPC isn't on our list of supported architectures if I remember correctly).
Updated•13 years ago
|
Keywords: sec-review-needed
Assignee | ||
Comment 15•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
Dunno if that's a correct fix but at least the build goes further with this on openbsd/sparc64.
Attachment #560774 -
Flags: review?(spectre)
Assignee | ||
Comment 18•13 years ago
|
||
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.
Attachment #560774 -
Flags: review?(spectre)
Comment 20•13 years ago
|
||
(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•13 years ago
|
||
(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...
Assignee | ||
Comment 22•13 years ago
|
||
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•13 years ago
|
||
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..
Updated•13 years ago
|
Attachment #560774 -
Flags: review?(dmandelin)
Assignee | ||
Comment 24•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
Attachment #560774 -
Attachment is obsolete: true
Attachment #560774 -
Flags: review?(dmandelin)
Attachment #561216 -
Flags: review?(dmandelin)
Comment 27•13 years ago
|
||
And for the record, it's https://bugzilla.mozilla.org/show_bug.cgi?id=687885
Comment 28•13 years ago
|
||
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!
Attachment #561216 -
Flags: review?(dmandelin) → review+
Assignee | ||
Comment 29•13 years ago
|
||
Requesting check-in to fix the (limited) bustage.
Keywords: checkin-needed
Updated•13 years ago
|
Attachment #559163 -
Flags: checkin+
Comment 30•13 years ago
|
||
Comment on attachment 561216 [details] [diff] [review] Remove inclusion of now removed jswtfbridge.h https://hg.mozilla.org/integration/mozilla-inbound/rev/c7233b484b95
Attachment #561216 -
Flags: checkin+
Updated•13 years ago
|
Keywords: checkin-needed
Comment 31•13 years ago
|
||
Comment on attachment 561216 [details] [diff] [review] Remove inclusion of now removed jswtfbridge.h https://hg.mozilla.org/mozilla-central/rev/c7233b484b95
Comment 33•13 years ago
|
||
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.
Assignee | ||
Comment 34•13 years ago
|
||
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•13 years ago
|
||
I believe bug 684039 may have broken this for you, see bug 684039 comment 46 for more info.
Assignee | ||
Comment 36•13 years ago
|
||
It appears so. It may be better for me just to turn things back to YARR interpreter for everyone, assuming no objections.
You need to log in
before you can comment on or make changes to this bug.
Description
•