[Webkit-unassigned] [Bug 38142] RegExp caching

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38142





--- Comment #20 from Geoffrey Garen <ggaren at apple.com>  2010-06-14 11:32:10 PST ---
+PassRefPtr<RegExp> RegExpCache::lookupOrCreate(const UString& patternString, const UString& flags)
+{
+    pair<HashMap<RegExpKey, RefPtr<RegExp> >::iterator, bool> result = m_cacheMap.add(RegExpKey(flags, patternString), 0);
+    if (patternString.size() < maxCacheablePatternLength && !result.second)
+        return (*result.first).second;

No need to check "patternString.size() < maxCacheablePatternLength" here. If an item is in the cache, it must, by logical deduction, satisfy the constraints of the cache.

However, you *should* check "patternString.size() < maxCacheablePatternLength" prior to calling HashMap::add, and avoid calling HashMap::add if the string is too long. One goal of maxCacheablePatternLength, I assume, is to avoid hashing very long strings. 

The C++ way to write "(*x).y" is "x->y".

+    ++m_ptr;

Let's give this a more descriptive name. How about "m_nextKeyToEvict"?

+    if (m_ptr >= maxCacheableEntries) {

Should be "m_ptr == maxCacheableEntries". m_ptr should never be > maxCacheableEntries.

+        // After this point the value of m_isFirstIteration will be false.
+        m_isFirstIteration = false;

I think the line of code is sufficient -- no need to add a comment with a duplicate meaning.

I guess what this flag really means is, "The cache is full, so a new addition must first evict something old." How about calling this variable "m_isFull", and reversing its value?

I think you misunderstood my comment about LRU. I'm not asking you to switch to LRU vs Round Robin. I don't have a strong opinion on an eviction strategy here.

r- because of the maxCacheablePatternLength bug, but I think the other suggestions here, and Oliver's suggestion, are important too.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.