[Webkit-unassigned] [Bug 38142] RegExp caching

[bugzilla-daemon at webkit.org]

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





--- Comment #16 from Renata Hodovan <hodovan.renata at stud.u-szeged.hu>  2010-06-02 03:31:48 PST ---
> This code would be much simpler if you changed RegExp::create to always take a flags argument, but do something efficient when it is null. That might be a good thing to do in a future patch.
Okay, I'm going to do it.

> +    if (patternString.size() < maxCacheablePatternLength) {
> +        ++m_ptr;
> +        if (m_ptr >= maxCacheableEntries) {
> +            m_ptr = 0;
> +            m_isFull = true;
> +        }
> +        if (m_isFull)
> +            m_cacheMap.remove(RegExpKey(patternKeyArray[m_ptr].flagsValue, patternKeyArray[m_ptr].pattern));
> +
> +        RegExpKey key = RegExpKey(flags, patternString);
> +        m_cacheMap.set(key, cachedRegExp);
> +        patternKeyArray[m_ptr].flagsValue = key.flagsValue;
> +        patternKeyArray[m_ptr].pattern = patternString.rep();
> +    }
> +    return cachedRegExp;
> 
> The WebKit style for code like this is to check for the failure case and return early. One advantage of this style is that less code ends up indented. So:
> 
> if (patternString.size() >= maxCacheablePatternLength)
>     return cachedRegExp;
> 
> I wouldn't call the regexp 'cachedRegExp', nor would I call the function 'createAndCache', since the regexp might not be cached. How about "regeExp" and "create" instead?
Thanks, I've changed it.

> You define m_isFull as a data member, but only use it locally in createAndCache. I think you can get rid of it entirely, and just rely on "if (m_ptr >= maxCacheableEntries)" in createAndCache.
m_isFull couldn't be local, because we have to know if the map became full and it isn't local inforamtion. But I think the name of the variable was abiguous, so I changed it to m_isFirstIteration.

> lookupOrCreate and createAndCache combine to do more hash lookups and RegExpKey construction than necessary. Here's what I'd recommend:
> 
> 1. lookupOrCreate should say "pair <iterator, bool> result = m_cacheMap.add(RegExpKey(flags, patternString), 0)"
> 2. if result.second is false, there was a pre-existing entry in the map. You can return it.
> 3. if result.second is true, you've added a 0 to the map, and result.first is an iterator pointing at that zero. You can now use that iterator to add a real value to the map.
That's right. Modified.

> I think that patternKeyArray is unnecessary complexity. I guess your goal is LRU eviction.
My implementation is Round Robin, not LRU. I have tried LRU before, but there was no perf. gain, and the code was more complex. However, if you prefer LRU model, I can submit that to the bugzilla.

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