If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dmandelin, Assigned: dmandelin)

Tracking

({fixed1.9.1})

Trunk
fixed1.9.1
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 348900 [details] [diff] [review]
Patch

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.

Updated

9 years ago
Severity: normal → enhancement
(Assignee)

Comment 1

9 years ago
Created attachment 351484 [details] [diff] [review]
Fixed patch

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)

Comment 2

9 years ago
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.
(Assignee)

Comment 3

9 years ago
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.

Comment 4

9 years ago
Yeah it doesn't solve it completely but its a good starting point. CCing vlad and stuart.
(Assignee)

Comment 5

9 years ago
Created attachment 360961 [details] [diff] [review]
Unrotted patch
Attachment #351484 - Attachment is obsolete: true
Attachment #360961 - Flags: review?(gal)
Attachment #351484 - Flags: review?(gal)

Updated

9 years ago
Attachment #360961 - Flags: review?(gal)
Attachment #360961 - Flags: review+
Attachment #360961 - Flags: approval1.9.1?

Comment 6

9 years ago
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.
(Assignee)

Comment 7

9 years ago
Pushed to TM as d55ca83118be.

Updated

9 years ago
Attachment #360961 - Flags: approval1.9.1? → approval1.9.1+

Comment 8

9 years ago
http://hg.mozilla.org/mozilla-central/rev/d55ca83118be
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 9

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5919bd3209d1
Keywords: fixed1.9.1
(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.

Updated

9 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.