[webkit-reviews] review denied: [Bug 61554] Make RegExpCache a weak map : [Attachment 95027] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 61554: Make RegExpCache a weak map
https://bugs.webkit.org/show_bug.cgi?id=61554

Attachment 95027: Patch
https://bugs.webkit.org/attachment.cgi?id=95027&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.