Summary: | Make RegExpCache a weak map | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Oliver Hunt <oliver> | ||||||||
Component: | New Bugs | Assignee: | Oliver Hunt <oliver> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | ||||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Oliver Hunt
2011-05-26 13:16:18 PDT
Created attachment 95027 [details]
Patch
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. Created attachment 95051 [details]
Patch
Created attachment 95055 [details]
Patch
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. Committed r87445: <http://trac.webkit.org/changeset/87445> |