Closed Bug 465656 Opened 11 years ago Closed 11 years ago

Native Regexps: compare flat strings word at a time when possible

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dmandelin, Assigned: dmandelin)

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
Flat strings of length >= 2 should be faster if we go word by word. In some cases this can even be done efficiently in case-insensitive mode. 

Attached patch does this, gives speedup of ~16ms on regexp-dna (down to about 51 ms), which is only 2 ms slower than my last WebKit build.

I need to test it more before anyone should review it, though.
Severity: normal → enhancement
Attached patch Fixed patch (obsolete) — Splinter Review
Here's the fixed patch. The original had a bug in case-insensitive mode for character pairs like '>P', where it's still OK to use word matching, but we have to be careful to mask only the first character and not the second.
Attachment #348900 - Attachment is obsolete: true
Attachment #351484 - Flags: review?(gal)
This got lost in my review queue. Is this tested?

We also realized that on arm this might be desperately needed since it can't do unaligned word access without an exception caught by the OS.
Yes, it's tested but I would retest after this length of time. The retest can go in parallel with review.

I'd like to hear more about the arm issue because if it is a problem I doubt this patch completely solves it.
Yeah it doesn't solve it completely but its a good starting point. CCing vlad and stuart.
Attached patch Unrotted patchSplinter Review
Attachment #351484 - Attachment is obsolete: true
Attachment #360961 - Flags: review?(gal)
Attachment #351484 - Flags: review?(gal)
Attachment #360961 - Flags: review?(gal)
Attachment #360961 - Flags: review+
Attachment #360961 - Flags: approval1.9.1?
Comment on attachment 360961 [details] [diff] [review]
Unrotted patch

I only see a 5ms speedup (or so), but we want this as basis for arm.
Pushed to TM as d55ca83118be.
Attachment #360961 - Flags: approval1.9.1? → approval1.9.1+
http://hg.mozilla.org/mozilla-central/rev/d55ca83118be
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(In reply to comment #6)
> (From update of attachment 360961 [details] [diff] [review])
> I only see a 5ms speedup (or so), but we want this as basis for arm.

Hmm, where we had to turn it off, because this actually makes the unaligned access worse -- you can read 16-bit values at a time on 16-bit alignment, you just can't read 32-bit values on non-32-bit alignment points.

I disabled this on WinCE at least in http://hg.mozilla.org/mozilla-central/rev/16cf92dc8c6c ; linux seems to trap the unaligned access and fixes it up, which can't be great for performance.
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.