HPCC-31921 Add caching of regex compiled search patterns #18748

dcamper · 2024-06-10T19:46:29Z

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

Manual execution of regression tests. Performance testing with pathological code (wall clock timings).

github-actions · 2024-06-10T19:46:49Z

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-31921

Jirabot Action Result:
Workflow Transition: Merge Pending
Updated PR

ghalliday

@dcamper looks good. One functional question, and a couple of performance comments.

ghalliday · 2024-06-13T05:18:15Z

system/jlib/jhash.hpp

+
+            recentList.erase(foundIter->second.second);
+            recentList.push_front(key);
+            lookupMap[key] = {foundIter->second.first, recentList.begin()};


Rather than replacing the entry you can just update the pointer in the LRU list.

foundIter->second.second = recentList.begin();

Good catch; fixed.

ghalliday · 2024-06-13T05:20:17Z

system/jlib/jhash.hpp

+            {
+                recentList.erase(foundIter->second.second);
+                recentList.push_front(key);
+                lookupMap[key] = {value, recentList.begin()};


foundIter->second = ...

is probably more efficient, but it shouldn't be hit in the current usage

Fixed.

Agreed that in the current regex scenario this block won't be hit. I really struggled with creating a general purpose feature at this level of code versus just what I needed, and reigning in my desire to add another version of this LRU cache that would be more efficient for non-pointer objects.

ghalliday · 2024-06-13T05:26:04Z

rtl/eclrtl/eclregex.cpp

+    // Check the cache
+    {
+        CriticalBlock lock(compiledStrRegExprLock);
+        compiledObjPtr = dynamic_cast<CCompiledStrRegExpr*>(compiledStrRegExprCache.get(regexHash));


What is the danger/likelihood of false positive matches caused by clashing hash codes?

I can do the math, but the likelihood is "very very low" considering cache size is only 500. It is never impossible to prevent collisions on a hash, but our key space is huge compared to the number of cached items.

Earlier versions of this code used separate caches for the three data types (string, UTF-8 and unicode). That would reduce the possibility of collision but not enough, IMO. Separate caches would also eliminate the need for the added parent class and the virtualization of destructors.

ghalliday · 2024-06-13T05:29:23Z

rtl/eclrtl/eclregex.cpp

    }

+    CCompiledStrRegExpr(CCompiledStrRegExpr & other) = default; // Note non-const argument


Why the need for the non-const argument?

The std::shared_ptr in other apparently needs to be updated (the compiler complained about the original const version I had). If you know of a pattern where I can make other const, please tell me!

Ah, never mind. I think I had a different underlying issue at the time. All copy constructors now accept const arguments.

ghalliday · 2024-06-13T05:31:49Z

ecl/hql/hqlfold.cpp

@@ -2796,7 +2796,7 @@ IHqlExpression * foldConstantOperator(IHqlExpression * expr, unsigned foldOption
                    StringBuffer pattern, search;
                    v0->getUTF8Value(pattern);
                    v1->getUTF8Value(search);
-                    ICompiledStrRegExpr * compiled = rtlCreateCompiledU8StrRegExpr(pattern, !expr->hasAttribute(noCaseAtom));
+                    ICompiledStrRegExpr * compiled = rtlCreateCompiledU8StrRegExpr(pattern.length(), pattern, !expr->hasAttribute(noCaseAtom));


This doesn't look right. I think it is passing a size to a length parameter.

Excellent catch; fixed.

ghalliday · 2024-06-13T05:39:27Z

rtl/eclrtl/eclregex.cpp

+    // Check the cache
+    {
+        CriticalBlock lock(compiledStrRegExprLock);
+        compiledObjPtr = dynamic_cast<CCompiledStrRegExpr*>(compiledStrRegExprCache.get(regexHash));


My recollection is that a dynamic cast is significantly less efficient than a static cast, or a virtual method call. It might be worth considering if it can be removed, but probably not worth it for this PR.

It is less efficient, but safer; I blame muscle memory for this one. I've swapped in static_cast because "I know what I'm doing" (hahaha). static_cast is resolved at compile time.

ghalliday

@dcamper looks good. My only concerns are
i) Please can you include a regression test (that is ok to be released open source) and with a smaller number of iterations.
ii) There is no way to configure on/off. I think that's probably ok.
iii) How likely are there to be clashes in hash codes. How much less efficient is a hash table that stores strings? The problem is that mismatches would be silent, and could cause chaos.

system/jlib/jhash.hpp

dcamper · 2024-06-18T14:01:25Z

iii) How likely are there to be clashes in hash codes. How much less efficient is a hash table that stores strings? The problem is that mismatches would be silent, and could cause chaos.

That cached object now retains the original regex pattern and the options. If the object is retrieved from the cache via the hash key, then the saved pattern and options are compared to ensure that there is no collision.

ghalliday

I think it looks functionally correct. A couple of suggestions to discuss about the structure.

ghalliday · 2024-06-19T10:49:28Z

system/jlib/jhash.hpp

+            if (foundIter == lookupMap.end())
+                return nullptr;
+
+            recentList.erase(foundIter->second.second);


recentList.splice(0, recentList, foundIter->second.second)

is probably more efficient - it should avoid cloning the list element. From the docs that form should be valid if source and target are the same.

Good catch. Changed here and in the set() method as well.

rtl/eclrtl/eclregex.cpp

ghalliday · 2024-06-19T11:11:50Z

rtl/eclrtl/eclregex.cpp

+ * <Software/Globals> section for an optional "regex" subsection with a "cacheSize" attribute
+ * By default, the maximum cache size is set to 500 patterns.  Override with 0 to disable caching.
+ */
+void initMaxCacheSize()


Add static to avoid possible namespace pollution

Good catch; fixed.

ghalliday

@dcamper looks very close. A few suggests to clean up the code, and I think this will currently leak RegexCacheEntry objects.

ghalliday · 2024-06-19T16:46:37Z

rtl/eclrtl/eclregex.cpp

+    : savedOptions(_options), savedPattern(_pattern, _patternSize), compiledRegex16(_compiledRegex16)
+    {}
+
+    RegexCacheEntry(const RegexCacheEntry & other) = default;


can now be = delete

ghalliday · 2024-06-19T16:46:47Z

rtl/eclrtl/eclregex.cpp

+
+    RegexCacheEntry(const RegexCacheEntry & other) = default;
+
+    virtual ~RegexCacheEntry() = default;


can now be removed.

ghalliday · 2024-06-19T16:49:57Z

rtl/eclrtl/eclregex.cpp


-        compiledRegex = pcre2_compile_8((PCRE2_SPTR8)_regex, regexSize, options, &errNum, &errOffset, pcre2CompileContext8);
+    CCompiledStrRegExpr(const CCompiledStrRegExpr & other) = default;


can now be = delete? or remove and just use the default.

ghalliday · 2024-06-19T16:50:52Z

rtl/eclrtl/eclregex.cpp

-            failWithPCRE2Error(errNum, "Error in regex pattern: ", _regex, errOffset);
-        }
-    }
+    virtual ~CCompiledStrRegExpr() = default;


can also probably now go away

ghalliday · 2024-06-19T16:52:57Z

rtl/eclrtl/eclregex.cpp

+    {
+        CCompiledStrRegExpr * compiledObjPtr = nullptr;
+        uint32_t options = (_isCaseSensitive ? 0 : PCRE2_CASELESS);
+        hash64_t regexHash = HASH64_INIT;


cleanup:
regexHash = RegexHashEntry::hashValue(_regexLength, _regex, options);
and elsewhere

ghalliday · 2024-06-19T16:57:20Z

system/jlib/jhash.hpp

+            while (lookupMap.size() > maxCacheSize)
+            {
+                lookupMap.erase(recentList.back());
+                recentList.pop_back();


When will the cache entry be deleted? Should it be a map to a std::unique_ptr?

ghalliday

@dcamper looks good and also a clean implementation. Thanks, please squash and I will merge.

dcamper · 2024-06-20T11:57:05Z

Clean implementation with a great deal of help. Thank you!

Rebased to latest master branch and squashed. Please merge.

dcamper requested a review from ghalliday June 10, 2024 19:46

dcamper force-pushed the hpcc-31921-regex-caching-master branch from e6c944b to da6cb86 Compare June 11, 2024 12:24

ghalliday reviewed Jun 13, 2024

View reviewed changes

dcamper requested a review from ghalliday June 13, 2024 12:34

ghalliday reviewed Jun 17, 2024

View reviewed changes

system/jlib/jhash.hpp Show resolved Hide resolved

dcamper requested a review from ghalliday June 18, 2024 15:56

ghalliday reviewed Jun 19, 2024

View reviewed changes

dcamper requested a review from ghalliday June 19, 2024 16:08

ghalliday reviewed Jun 19, 2024

View reviewed changes

dcamper force-pushed the hpcc-31921-regex-caching-master branch from 1d747f5 to 261c5f3 Compare June 19, 2024 17:02

dcamper requested a review from ghalliday June 19, 2024 18:33

ghalliday approved these changes Jun 20, 2024

View reviewed changes

dcamper force-pushed the hpcc-31921-regex-caching-master branch from fae681d to 7637764 Compare June 20, 2024 11:54

HPCC-31921 Add caching of regex compiled search patterns

7997a0a

dcamper force-pushed the hpcc-31921-regex-caching-master branch from 7637764 to 7997a0a Compare June 20, 2024 11:55

ghalliday merged commit 683bc67 into hpcc-systems:master Jun 20, 2024
50 of 52 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-31921 Add caching of regex compiled search patterns #18748

HPCC-31921 Add caching of regex compiled search patterns #18748

dcamper commented Jun 10, 2024

github-actions bot commented Jun 10, 2024

ghalliday left a comment

ghalliday Jun 13, 2024

dcamper Jun 13, 2024

ghalliday Jun 13, 2024

dcamper Jun 13, 2024

ghalliday Jun 13, 2024

dcamper Jun 13, 2024

ghalliday Jun 13, 2024

dcamper Jun 13, 2024

dcamper Jun 13, 2024

ghalliday Jun 13, 2024

dcamper Jun 13, 2024

ghalliday Jun 13, 2024

dcamper Jun 13, 2024

ghalliday left a comment

dcamper commented Jun 18, 2024

ghalliday left a comment

ghalliday Jun 19, 2024

dcamper Jun 19, 2024

ghalliday Jun 19, 2024

dcamper Jun 19, 2024

ghalliday left a comment

ghalliday Jun 19, 2024

ghalliday Jun 19, 2024

ghalliday Jun 19, 2024

ghalliday Jun 19, 2024

ghalliday Jun 19, 2024

ghalliday Jun 19, 2024

ghalliday left a comment

dcamper commented Jun 20, 2024

		}

		CCompiledStrRegExpr(CCompiledStrRegExpr & other) = default; // Note non-const argument


		RegexCacheEntry(const RegexCacheEntry & other) = default;

		virtual ~RegexCacheEntry() = default;


		compiledRegex = pcre2_compile_8((PCRE2_SPTR8)_regex, regexSize, options, &errNum, &errOffset, pcre2CompileContext8);
		CCompiledStrRegExpr(const CCompiledStrRegExpr & other) = default;

HPCC-31921 Add caching of regex compiled search patterns #18748

HPCC-31921 Add caching of regex compiled search patterns #18748

Conversation

dcamper commented Jun 10, 2024

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Jun 10, 2024

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

dcamper commented Jun 18, 2024

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

dcamper commented Jun 20, 2024