Bug 15749 - RegExp/RegExpObjectImp cause needless UString creation
Summary: RegExp/RegExpObjectImp cause needless UString creation
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:
 
Reported: 2007-10-29 02:45 PDT by Eric Seidel (no email)
Modified: 2007-10-31 07:58 PDT (History)
3 users (show)

See Also:


Attachments
patch (21.15 KB, patch)
2007-10-30 21:54 PDT, Darin Adler
mjs: 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-10-29 02:45:38 PDT
RegExp/RegExpObjectImp cause needless UString creation

performMatch/match return a UString, which is only used by very few callers (and I'm not sure those are even necessary).  Instead it just needs to return the size of the match.  if the size is non-zero, then there was a match (what most callers care about) and the callers which care about the size have that too.  There are a couple cases (replacement) where we need to actually be able to generate the match string, but that can be done using the returned offset vector and some sort of helper function.  I expect this will be a couple percentage points speedup, due to the reduce malloc pressure.

I'm too tired to write it (and busy much of tomorrow).  Anyone who feels the urge should feel free.
Comment 1 Darin Adler 2007-10-30 21:54:27 PDT
Created attachment 16958 [details]
patch
Comment 2 Eric Seidel (no email) 2007-10-30 22:37:18 PDT
Comment on attachment 16958 [details]
patch

Hum... I wonder if there isn't a clean way to use a local OwnArrayPtr to make sure that any early exit cleans up the storage... meaning, only calling localPtr.release() and setting ovector at the very end of the success case.  You'd of course need a local raw ptr which pointed at either at the stack buffer (of size 3) or the newly allocated localPtr's array.  I'm not sure that would be any less complex than the current solution though.  I guess this is sorta what vector is for.

Anyway, sorry not enough brain cells for a full review.  Slightly surprised it wasn't more of a gain (since in at least some tests this was a major cause of UString creation).
Comment 3 Maciej Stachowiak 2007-10-30 23:03:06 PDT
Comment on attachment 16958 [details]
patch

r=me
Comment 4 Darin Adler 2007-10-31 07:51:53 PDT
(In reply to comment #2)
> I guess this is sorta what vector is for.

I toyed with multiple implementations. The first thing I tried was Vector<int>. But it was not more elegant, and in fact it was less efficient since a Vector has overhead to allow for resizing. So I went with this.
Comment 5 Darin Adler 2007-10-31 07:58:11 PDT
Committed revision 27320.