Bug 61554

Summary: Make RegExpCache a weak map
Product: WebKit Reporter: Oliver Hunt <oliver>
Component: New BugsAssignee: Oliver Hunt <oliver>
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch ggaren: review+

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]
Comment 2 Geoffrey Garen 2011-05-26 14:29:39 PDT
Comment on attachment 95027 [details]

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]
Comment 4 Oliver Hunt 2011-05-26 15:18:30 PDT
Created attachment 95055 [details]
Comment 5 Geoffrey Garen 2011-05-26 15:23:22 PDT
Comment on attachment 95055 [details]

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


> 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>