Bug 61554 - Make RegExpCache a weak map
Summary: Make RegExpCache a weak map
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-26 13:16 PDT by Oliver Hunt
Modified: 2011-05-26 15:59 PDT (History)
0 users

See Also:


Attachments
Patch (13.47 KB, patch)
2011-05-26 13:20 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (14.04 KB, patch)
2011-05-26 15:03 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (13.52 KB, patch)
2011-05-26 15:18 PDT, Oliver Hunt
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2011-05-26 13:16:18 PDT
Make RegExpCache a weak map
Comment 1 Oliver Hunt 2011-05-26 13:20:44 PDT
Created attachment 95027 [details]
Patch
Comment 2 Geoffrey Garen 2011-05-26 14:29:39 PDT
Comment on attachment 95027 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95027&action=review

It's good to reuse an expression as long as it's alive. But in generational GC, this implementation will cause regexps to live super long, since we'll stop asking "isReachable" after a while. So, number of GC's seen isn't a very good measure of time.

I'd suggest this two-pronged solution:

(1) Once a fixed limit of expressions is reached, the cache should stop keeping anything artificially alive, and return false from "isReachable." This means that expressions will naturally exit the cache as they become unreachable, but will continue to be shared if they are reachable.

(2) When the system comes under executable memory pressure, iterate the cache and throw all regexp code away. The nice thing about this is that executable memory footprint will never get out of hand. (This can be a separate patch.)

> Source/JavaScriptCore/runtime/RegExpCache.cpp:41
> +    RegExp* regexp = new (m_globalData) RegExp(m_globalData, patternString, flags);

Could use a comment here explaining that GC might modify m_cacheMap.

> Source/JavaScriptCore/runtime/RegExpCache.cpp:58
> +    if (regExp->hasCode() && regExp->gcShouldInvalidateCode())
> +        regExp->invalidateCode();

This kind of operation belongs in finalize, not isReachable.
Comment 3 Oliver Hunt 2011-05-26 15:03:20 PDT
Created attachment 95051 [details]
Patch
Comment 4 Oliver Hunt 2011-05-26 15:18:30 PDT
Created attachment 95055 [details]
Patch
Comment 5 Geoffrey Garen 2011-05-26 15:23:22 PDT
Comment on attachment 95055 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95055&action=review

r=me

> Source/JavaScriptCore/runtime/RegExpCache.h:60
>      RegExpCacheMap m_cacheMap;
> +    int m_nextEntryInStrongCache;
> +    WTF::FixedArray<Strong<RegExp>, maxStrongCacheableEntries> m_strongCache;

Since you've called the new cache the strong cache, I'd call the other cache m_weakCache, and a comment for each:
m_strongCache; // Holds a select few regular expressions that have compiled and executed
m_weakCache; // Holds all regular expressions currently live.
Comment 6 Oliver Hunt 2011-05-26 15:59:42 PDT
Committed r87445: <http://trac.webkit.org/changeset/87445>