Bug 81742 - RegExpMatchesArray should not copy the ovector
Summary: RegExpMatchesArray should not copy the ovector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-20 22:19 PDT by Gavin Barraclough
Modified: 2012-03-24 12:09 PDT (History)
1 user (show)

See Also:


Attachments
Fix (35.76 KB, patch)
2012-03-20 22:21 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2012-03-20 22:19:40 PDT
Currently, all RegExpMatchesArray object contain Vector<int, 32>, used to hold any sub-pattern results.  This makes allocation/construction/destruction of these objects more expensive.  Instead, just store the main match, and recreate the sub-pattern ranges only if necessary (these are often only used for grouping, and the results never accessed).  If the main match (index 0) of the RegExpMatchesArray is accessed, reify that value alone.
Comment 1 Gavin Barraclough 2012-03-20 22:21:08 PDT
Created attachment 132972 [details]
Fix
Comment 2 WebKit Review Bot 2012-03-20 22:23:40 PDT
Attachment 132972 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/runtime/RegExpObject.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/RegExpObject.h:120:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Michael Saboff 2012-03-21 11:00:04 PDT
Comment on attachment 132972 [details]
Fix

r+

Please fix the style issues.

What is the performance implication of this change?
Comment 4 Gavin Barraclough 2012-03-21 14:04:28 PDT
Fixed in r111603.
Comment 5 Oliver Hunt 2012-03-24 12:09:58 PDT
Comment on attachment 132972 [details]
Fix

Clearing review flags as comments imply this has landed.