Add cache for RegExp.test #3802

MikeHolman · 2017-09-25T20:39:20Z

It seems to be a common enough pattern where people will create a RegExp and test a small number of unique strings many times.

For this, I added a simple cache of size 8 on the RegExp object for RegExp.test, which caches the result for some given input string.

I disable the cache it in case of using global or sticky, since the result can change depending on lastIndex and caching likely wouldn't help those patterns.

After experimentation I decided on size 8, because there were rapidly diminishing returns at size 16 and at size 4 I saw a big increase in cache misses.

I tried dynamic sizing as well, but megamorphic cases would quickly max out the cache and the polymorphic cases didn't seem to get much benefit above size 8, so it seemed better to avoid the overhead of resizing and stick with a fixed sized cache.

Improves perf of angular by about 6%.

agarwal-sandeep · 2017-09-25T21:03:12Z

lib/Runtime/Library/RegexHelper.cpp

+
+        if (useCache && cachedValue == input)
+        {
+            return JavascriptBoolean::ToVar(cache[cacheIndex].result, scriptContext);


return JavascriptBoolean::ToVar(cache[cacheIndex].result, scriptContext); [](start = 12, length = 73)

In case of a cache hit Is it still worth to do the slow lookup in chk build and validate against cached value?

seems like a good idea

rajatd · 2017-09-25T21:25:20Z

lib/Runtime/Library/JavascriptRegularExpression.h

+    struct RegExpTestCache
+    {
+        bool result;
+        Field(JavascriptString*) input;


What do you think about making this a weak reference to the string?

I originally tried this and had problems due to ArenaLiteralStrings. These are only used in a couple of places and really don't seem that useful, so I decided to remove them altogether and replace them with normal recycler managed strings.

agarwal-sandeep · 2017-09-26T17:03:07Z

lib/Runtime/Library/RegexHelper.cpp

-        const bool isGlobal = pattern->IsGlobal();
-        const bool isSticky = pattern->IsSticky();
+
+        if (useCache && cacheHit)


useCache && [](start = 12, length = 11)

nit: no need of this as cacheHit can only be true if useCache was true.

agarwal-sandeep

Merge pull request #3802 from MikeHolman:regextest It seems to be a common enough pattern where people will create a RegExp and test a small number of unique strings many times. For this, I added a simple cache of size 8 on the RegExp object for RegExp.test, which caches the result for some given input string. I disable the cache it in case of using global or sticky, since the result can change depending on lastIndex and caching likely wouldn't help those patterns. After experimentation I decided on size 8, because there were rapidly diminishing returns at size 16 and at size 4 I saw a big increase in cache misses. I tried dynamic sizing as well, but megamorphic cases would quickly max out the cache and the polymorphic cases didn't seem to get much benefit above size 8, so it seemed better to avoid the overhead of resizing and stick with a fixed sized cache. Improves perf of angular by about 6%.

…atches and used a cached value Regression from chakra-core#3802 - if we hit the cache, we don't update the Regex constructor object with the last match and so retrieving properties about the match from the regex constructor object fails. Fixed by adding an invalidation mechanism to the `JavascriptRegExpConstructor`. If we hit the cache, we'll mark the Regex constructor object last match properties as invalidated and we will then compute them on-demand the first time they're needed. Fixes: https://microsoft.visualstudio.com/web/wi.aspx?id=14763260

…r RegExp.prototype.test matches and used a cached value Merge pull request #4607 from boingoing:FixRegExpTestCache Regression from #3802 - if we hit the cache, we don't update the Regex constructor object with the last match and so retrieving properties about the match from the regex constructor object fails. Fixed by adding an invalidation mechanism to the `JavascriptRegExpConstructor`. If we hit the cache, we'll mark the Regex constructor object last match properties as invalidated and we will then compute them on-demand the first time they're needed. Fixes: https://microsoft.visualstudio.com/web/wi.aspx?id=14763260

…xp.$1 after RegExp.prototype.test matches and used a cached value Merge pull request #4607 from boingoing:FixRegExpTestCache Regression from #3802 - if we hit the cache, we don't update the Regex constructor object with the last match and so retrieving properties about the match from the regex constructor object fails. Fixed by adding an invalidation mechanism to the `JavascriptRegExpConstructor`. If we hit the cache, we'll mark the Regex constructor object last match properties as invalidated and we will then compute them on-demand the first time they're needed. Fixes: https://microsoft.visualstudio.com/web/wi.aspx?id=14763260

…y update RegExp.$1 after RegExp.prototype.test matches and used a cached value Merge pull request #4607 from boingoing:FixRegExpTestCache Regression from #3802 - if we hit the cache, we don't update the Regex constructor object with the last match and so retrieving properties about the match from the regex constructor object fails. Fixed by adding an invalidation mechanism to the `JavascriptRegExpConstructor`. If we hit the cache, we'll mark the Regex constructor object last match properties as invalidated and we will then compute them on-demand the first time they're needed. Fixes: https://microsoft.visualstudio.com/web/wi.aspx?id=14763260

@boingoing

[1.8>1.9] [MERGE #4607 @boingoing] OS#14763260: Correctly update RegExp.$1 after RegExp.prototype.test matches and used a cached value Merge pull request #4607 from boingoing:FixRegExpTestCache Regression from chakra-core/ChakraCore#3802 - if we hit the cache, we don't update the Regex constructor object with the last match and so retrieving properties about the match from the regex constructor object fails. Fixed by adding an invalidation mechanism to the `JavascriptRegExpConstructor`. If we hit the cache, we'll mark the Regex constructor object last match properties as invalidated and we will then compute them on-demand the first time they're needed. Fixes: https://microsoft.visualstudio.com/web/wi.aspx?id=14763260 Reviewed-By: chakrabot <chakrabot@users.noreply.github.com>

@boingoing

[1.9>master] [1.8>1.9] [MERGE #4607 @boingoing] OS#14763260: Correctly update RegExp.$1 after RegExp.prototype.test matches and used a cached value Merge pull request #4607 from boingoing:FixRegExpTestCache Regression from chakra-core/ChakraCore#3802 - if we hit the cache, we don't update the Regex constructor object with the last match and so retrieving properties about the match from the regex constructor object fails. Fixed by adding an invalidation mechanism to the `JavascriptRegExpConstructor`. If we hit the cache, we'll mark the Regex constructor object last match properties as invalidated and we will then compute them on-demand the first time they're needed. Fixes: https://microsoft.visualstudio.com/web/wi.aspx?id=14763260 Reviewed-By: chakrabot <chakrabot@users.noreply.github.com>

@boingoing

[1.8>1.9] [MERGE #4607 @boingoing] OS#14763260: Correctly update RegExp.$1 after RegExp.prototype.test matches and used a cached value Merge pull request #4607 from boingoing:FixRegExpTestCache Regression from chakra-core/ChakraCore#3802 - if we hit the cache, we don't update the Regex constructor object with the last match and so retrieving properties about the match from the regex constructor object fails. Fixed by adding an invalidation mechanism to the `JavascriptRegExpConstructor`. If we hit the cache, we'll mark the Regex constructor object last match properties as invalidated and we will then compute them on-demand the first time they're needed. Fixes: https://microsoft.visualstudio.com/web/wi.aspx?id=14763260 Reviewed-By: chakrabot <chakrabot@users.noreply.github.com>

msftclas added the cla-not-required label Sep 25, 2017

MikeHolman requested a review from rajatd September 25, 2017 20:48

agarwal-sandeep reviewed Sep 25, 2017

View reviewed changes

rajatd reviewed Sep 25, 2017

View reviewed changes

MikeHolman force-pushed the regextest branch from 352b260 to d4c10fb Compare September 25, 2017 23:56

msftgits removed the cla-not-required label Sep 26, 2017

agarwal-sandeep reviewed Sep 26, 2017

View reviewed changes

agarwal-sandeep approved these changes Sep 26, 2017

View reviewed changes

rajatd approved these changes Sep 26, 2017

View reviewed changes

MikeHolman added 5 commits September 26, 2017 13:25

add cache for regex test

bf0454b

still do real test on debug builds to validate cache

ae882dd

use weakref to string

5f6876f

remove ArenaLiteralString

50fc14e

address CR

94491e8

MikeHolman force-pushed the regextest branch from 3834be2 to 94491e8 Compare September 26, 2017 20:27

chakrabot merged commit 94491e8 into chakra-core:master Sep 26, 2017

boingoing mentioned this pull request Jan 27, 2018

OS#14763260: Correctly update RegExp.$1 after RegExp.prototype.test matches and used a cached value #4607

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add cache for RegExp.test #3802

Add cache for RegExp.test #3802

Uh oh!

MikeHolman commented Sep 25, 2017

Uh oh!

agarwal-sandeep Sep 25, 2017

Uh oh!

MikeHolman Sep 25, 2017

Uh oh!

rajatd Sep 25, 2017

Uh oh!

MikeHolman Sep 25, 2017 •

edited

Loading

Uh oh!

agarwal-sandeep Sep 26, 2017

Uh oh!

agarwal-sandeep left a comment

Uh oh!

Uh oh!

Add cache for RegExp.test #3802

Add cache for RegExp.test #3802

Uh oh!

Conversation

MikeHolman commented Sep 25, 2017

Uh oh!

agarwal-sandeep Sep 25, 2017

Choose a reason for hiding this comment

Uh oh!

MikeHolman Sep 25, 2017

Choose a reason for hiding this comment

Uh oh!

rajatd Sep 25, 2017

Choose a reason for hiding this comment

Uh oh!

MikeHolman Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agarwal-sandeep Sep 26, 2017

Choose a reason for hiding this comment

Uh oh!

agarwal-sandeep left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MikeHolman Sep 25, 2017 •

edited

Loading