Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(141)

Issue 8104: Regexp caching (Closed)

Created:
12 years, 2 months ago by Christian Plesner Hansen
Modified:
9 years, 7 months ago
Reviewers:
Kasper Lund
CC:
v8-dev
Visibility:
Public.

Description

- Added caching of regexp data in the compilation cache. - Changed the structure of regexp objects from having two internal fields to having a single field containing a fixed array, since it's easier to store the whole fixed array in the cache. - Move printing of the command to after printing std{err,out} in the compact progress indicators in the test framework.

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+228 lines, -77 lines) Patch
M src/compilation-cache.h View 2 chunks +10 lines, -1 line 3 comments Download
M src/compilation-cache.cc View 2 chunks +17 lines, -1 line 4 comments Download
M src/factory.h View 1 chunk +6 lines, -0 lines 1 comment Download
M src/factory.cc View 1 chunk +14 lines, -0 lines 0 comments Download
M src/jsregexp.h View 2 chunks +3 lines, -2 lines 0 comments Download
M src/jsregexp.cc View 10 chunks +64 lines, -37 lines 2 comments Download
M src/log.h View 1 chunk +1 line, -1 line 0 comments Download
src/log.cc View 2 chunks +8 lines, -10 lines 1 comment Download
M src/objects.h View 3 chunks +17 lines, -7 lines 1 comment Download
M src/objects.cc View 3 chunks +65 lines, -0 lines 2 comments Download
M src/objects-debug.cc View 1 chunk +15 lines, -12 lines 1 comment Download
M src/objects-inl.h View 1 chunk +7 lines, -5 lines 1 comment Download
M tools/test.py View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 2 (0 generated)
Christian Plesner Hansen
12 years, 2 months ago (2008-10-23 15:46:11 UTC) #1
Kasper Lund
12 years, 2 months ago (2008-10-24 06:42:50 UTC) #2
LGTM with some comments:

http://codereview.chromium.org/8104/diff/1/2
File src/compilation-cache.cc (right):

http://codereview.chromium.org/8104/diff/1/2#newcode35
Line 35: NUMBER_OF_ENTRY_KINDS = CompilationCache::_LAST_ENTRY
LAST_ENTRY + 1

http://codereview.chromium.org/8104/diff/1/2#newcode147
Line 147: Handle<CompilationCacheTable> table = GetTable(REGEXP);
Shouldn't this be tracking compilation cache misses and hits like the other
lookup functions?

http://codereview.chromium.org/8104/diff/1/2#newcode147
Line 147: Handle<CompilationCacheTable> table = GetTable(REGEXP);
Be careful not to leak the table handle into the surrounding handle scope. You
should have a HandleScope here like in the generic static Lookup function.

http://codereview.chromium.org/8104/diff/1/2#newcode148
Line 148: return Handle<Object>(table->LookupRegExp(*source, flags));
The return value is different from the other lookup functions (they return empty
handles on miss). Is there a good reason for this?

http://codereview.chromium.org/8104/diff/1/3
File src/compilation-cache.h (right):

http://codereview.chromium.org/8104/diff/1/3#newcode47
Line 47: _LAST_ENTRY
I would change _LAST_ENTRY to LAST_ENTRY and let it be equal to REGEXP: enum
Entry { ..., LAST_ENTRY = REGEXP }. The LAST_ENTRY should be the last - not an
additional one. Then you also avoid dealing with it in a switch...

http://codereview.chromium.org/8104/diff/1/3#newcode64
Line 64: static Handle<Object> LookupRegExp(Handle<String> source,
Maybe you should explain return value in case of a cache miss?

http://codereview.chromium.org/8104/diff/1/3#newcode67
Line 67: static void PutRegExp(Handle<String> source,
Maybe you should rename the Associate method to PutFunction to make it
consistent with PutRegExp? Maybe you should have a comment here that explains
what happens with existing mappings?

http://codereview.chromium.org/8104/diff/1/5
File src/factory.h (right):

http://codereview.chromium.org/8104/diff/1/5#newcode311
Line 311: static void SetRegExpData(Handle<JSRegExp> regexp,
Maybe a comment here? Does it set the type, source, flags, and data on the given
regexp? I guess so.

http://codereview.chromium.org/8104/diff/1/6
File src/jsregexp.cc (right):

http://codereview.chromium.org/8104/diff/1/6#newcode148
Line 148: int flags = JSRegExp::NONE;
Would it make sense to have some sort of opaque data type for RegExp flags?
Using 'int' seems a bit too generic. Sometimes we use an enum with no entries;
see Code::Flags.

http://codereview.chromium.org/8104/diff/1/6#newcode174
Line 174: bool in_cache = cached->IsFixedArray();
I would move the IsFixedArray logic into the cache to make it more consistent
with the other CompilationCache lookup functions and change the check here to an
empty handle check. Then you could also change the return type of LookupRegExp
to Handle<FixedArray>.

http://codereview.chromium.org/8104/diff/1/8
File src/log.cc (right):

http://codereview.chromium.org/8104/diff/1/8#newcode381
Line 381: case JSRegExp::ATOM:
Fix switch indentation. The cases should be indented with two spaces.

http://codereview.chromium.org/8104/diff/1/10
File src/objects-debug.cc (right):

http://codereview.chromium.org/8104/diff/1/10#newcode664
Line 664: case JSRegExp::ATOM: {
Indent switch cases by two spaces.

http://codereview.chromium.org/8104/diff/1/11
File src/objects-inl.h (right):

http://codereview.chromium.org/8104/diff/1/11#newcode2180
Line 2180: return FixedArray::cast(data())->get(index);
Maybe you should add an ASSERT that data isn't undefined or that the TypeTag()
returns something not NOT_COMPILED. I know the FixedArray::cast will give you
the same, but is seems more direct.

http://codereview.chromium.org/8104/diff/1/12
File src/objects.cc (right):

http://codereview.chromium.org/8104/diff/1/12#newcode5554
Line 5554: // RegExpKey carries a regexp object as key.
Is this really so? It seems to hold a string and some flags?

http://codereview.chromium.org/8104/diff/1/12#newcode5578
Line 5578: static uint32_t RegExpObjHash(Object* obj) {
I don't like the Obj abbreviation here. RegExpObjectHash?

http://codereview.chromium.org/8104/diff/1/13
File src/objects.h (right):

http://codereview.chromium.org/8104/diff/1/13#newcode1862
Line 1862: Object* LookupRegExp(String* source, int flags);
source -> src for consistency... or change src -> source.

Powered by Google App Engine
This is Rietveld 408576698