351487 - RegExp: Branch-callback checking could be made slightly more optimal

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Crowder]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a1) Gecko/20060823 Minefield/3.0a1
Build Identifier: 

The patch to bug #307456 implemented a branch callback check in spidermonkey's regular expression engine.  This check executes at least partially everytime it encounters a REOP_REPEAT or a REOP_REPEATMINIMAL.  The check could instead be deferred to happen only when the RE engine backtracks.  Patch being submitted shortly.  Adhoc benchmarks reveal at least a small performance increase for a few regexes I had laying around.

Reproducible: Always

Comment 1

[Brian Crowder]

Attachment: Tiny performance tweak for regex

Comment 2

[Brian Crowder]

Lowering severity to trivial.

Comment 3

[Brian Crowder]

We might consider tweaking the 127 magic number down to 63 (or less, perhaps?), since this change could result in the branch callback happening too infrequently to a responsive experience in the browser.

Comment 4

[Brian Crowder]

Nevermind.  Brendan tells me the branch callback in FF concerns itself with wall-time, not a counter, so maybe we should aim for & 511, or & 1023 here.  I will experiment.

Comment 5

[Brian Crowder]

Attachment: Use CHECK_BRANCH macro less

Other than changing the mask (from 127 to 0xffff), the only changes to the macro itself are cosmetic (pulled the end-of-line '\' in one character).  I also moved the macro itself closer to the code where it is now used.  It is now only used in this one place, so I was torn between just inserting the code there directly or keeping the old macro.  If there is a stylistic preference either way, let me know.  This patch significantly reduces the number of invocations of the branch-callback and in my very adhoc testing seems to shave at least 2 or 3 tenths of a second off of the resolution of a ~9.5 second regex execution.  I believe the slight speed-up should help also with expressions that aren't exponential; I imagine those will rarely if ever invoke the branch callback now, unless the input string is very large.

Comment 6

[Brian Crowder]

Comment on attachment 236884 [details] [diff] [review]
Tiny performance tweak for regex

Obsoleting this, since I forgot to obsolete it before.  mrbkap you reviewed the later of the two patches, yes?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #6)
> (From update of attachment 236884 [details] [diff] [review] [edit])
> Obsoleting this, since I forgot to obsolete it before.  mrbkap you reviewed the
> later of the two patches, yes?

Yeah, I reviewed the 2nd patch (see the attachment flags).

Comment 8

[Brian Crowder]

Comment on attachment 237183 [details] [diff] [review]
Use CHECK_BRANCH macro less

Taking brendan off sr? since I think this is pretty straightforward and mrbkap is happy (and brendan is busy)

Comment 9

[Brian Crowder]

Attachment: bitrot fixes

Dunno if this needs re-review or not, but why not be safe?

Comment 10

[Brian Crowder]

timeless landed this, I'm happy.