Bug 16185 - jsRegExpCompile should not add implicit non-capturing bracket
Summary: jsRegExpCompile should not add implicit non-capturing bracket
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks: 16186
  Show dependency treegraph
 
Reported: 2007-11-29 04:07 PST by Eric Seidel (no email)
Modified: 2007-12-07 12:01 PST (History)
2 users (show)

See Also:


Attachments
patch to omit the outer bracket in cases where it's not needed (9.75 KB, patch)
2007-12-01 10:41 PST, Darin Adler
no flags Details | Formatted Diff | Diff
patch (11.58 KB, patch)
2007-12-07 01:29 PST, Darin Adler
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2007-11-29 04:07:16 PST
jsRegExpCompile & match should not add implicit non-capturing bracket

Every regexp gets an implicit non-capturing bracket added to the front, and a non-capturing close added to the back of the instruction stream in jsRegExpCompile, match() is then called and expects these.

Removing this trip through the switch() would be a big savings on SunSpider.
Comment 1 Eric Seidel (no email) 2007-11-29 04:08:34 PST
To fix this would likely require breaking out match() into match() and recursive_match() and making sure that only outer match() function is ever called by callers, and that recursive_match() is only ever called from match().

I expect this would fall into the "softball" category of JS speedups.
Comment 2 Darin Adler 2007-12-01 10:40:25 PST
So I misunderstood this bug and added code so that jsRegExpCompile will omit the outer bracket entirely when it's not needed.

This doesn't help the regexp-dna test in SunSpider much at all, because almost all its regular expressions have "|" characters in them, which means they *do* require the outer bracket.

Hence Eric's other suggestion, of optimizing match for this, really turns into a sort of "unrolling" of the match function, which could indeed be helpful for simple regular expressions.

I can't help thinking it's a little bit too specific to this particular test, though. The more I look at the profile the more I realize it's a little bit too biased toward this one particular set of regular expressions.
Comment 3 Darin Adler 2007-12-01 10:41:22 PST
Created attachment 17628 [details]
patch to omit the outer bracket in cases where it's not needed
Comment 4 Darin Adler 2007-12-07 01:29:41 PST
Created attachment 17768 [details]
patch
Comment 5 Geoffrey Garen 2007-12-07 11:38:23 PST
Comment on attachment 17768 [details]
patch

r=me
Comment 6 Darin Adler 2007-12-07 12:01:35 PST
Committed revision 28525.