Uh oh!
There was an error while loading. Please reload this page.
Improve perf and scalability of Regex's cache - #542
Conversation
Regex maintains a cache used for the static methods on Regex, e.g. Regex.IsMatch. The cache is implemented as an LRU cache, which maintains a linked list and a dictionary of the cached instances. The linked list maintains the order in which the cached instances were last accessed, making it cheap to expunge older items from the cache. However, that comes at a significant cost: unless the item is the very first one in the linked list, all reads on the cache require taking a global lock, because the linked list needs to be mutated to move the found node to the beginning. That lock has both throughput and scalability implications. This PR changes the cache from using a `Dictionary<>` and a linked list to instead using a `ConcurrentDictionary<>` and a `List<>`. Rather than making all accesses more expensive in order to make drops less expensive, it makes all reads much cheaper and more scalable, at the expense of making drops more expensive. Since dropping from the cache means we're already paying the expensive cost of creating/parsing/compiling/etc. a new Regex instance, this is a better trade-off, especially since any frequent dropping suggests the consuming app or library needs to revisit its Regex strategy, either using Regex.CacheSize to increase the cache size appropriately, or doing its own caching (e.g. creating the Regex instance it needs and storing it into a field for all future use). The new scheme uses a `ConcurrentDictionary<Key,Node>`, a `List<Node>`, and a fast-path field storing the most recently used Regex instance (just as the existing implementation did). On lookups, if the fast-path field has the matching value, it's just returned. Otherwise, the dictionary is consulted, and if the item is found, the fast-path field is updated. No locking at all is employed, and only a few volatile read/writes are used to update a "last access stamp" that's used to indicate importance if/when items do need to be expunged. On additions, we do still take a global lock and add to the cache. If this puts us over our cache size, we pick an item from the list and remove it. If the list is small, we just examine all of the items looking for the oldest. If the list is larger, we examine a random subset of it; we may not get rid of the absolute oldest item, but it'll be old enough.
stephentoub
commented
Dec 5, 2019
The CI failures here are strange; lots of EventSource tests failing with, e.g. My assumption is that a) there was some kind of change in coreclr recently that is now causing a discrepancy with the tests, and b) this is now showing up after @safern's live/live change went in last night, but I'm not sure why I don't see similar failures on other PRs, nor why the "Actual" string above looks corrupted ("System.Collections.Concurrent.ConcurrentCúúú"). Regardless, I put up #565 to add this EventSource to the test's exempted list. @noahfalk, ideas? |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
safern
commented
Dec 5, 2019
Does this repro locally with and without your change? |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
eerhardt
left a comment
There was a problem hiding this comment.
This looks good to me. Just some clarifying questions to help me understand.
No. And CI passed now. |
noahfalk
commented
Jan 3, 2020
@stephentoub - sorry for a very late reply, I've been on vacation all December and GitHub doesn't have a nice out-of-office feature. I could imagine that your usage of ConcurrentDictionary caused the ConcurrentCollectionsEventSource to get lazily created in a bunch of tests that previously never initialized it, which in turn caused it to get flagged by the test code which is asserting that no unexpected EventSources had been created. Adding it to the exclusion list of known BCL EventSources was the right move. As for why the string was showing up corrupted, that I can't explain. I think its much more likely that it is some issue relating to xunit or the console display given that the string comparison you added at line 39 would only work if eventSource.Name contained the expected string data at that point. |
stephentoub
commented
Jan 6, 2020
Thanks, Noah. |
danmoseley
commented
Jan 6, 2020
Maybe |
Regex maintains a cache used for the static methods on Regex, e.g. Regex.IsMatch. The cache is implemented as an LRU cache, which maintains a linked list and a dictionary of the cached instances. The linked list maintains the order in which the cached instances were last accessed, making it cheap to expunge older items from the cache. However, that comes at a significant cost: unless the item is the very first one in the linked list, all reads on the cache require taking a global lock, because the linked list needs to be mutated to move the found node to the beginning. That lock has both throughput and scalability implications.
This PR changes the cache from using a
Dictionary<>and a linked list to instead using aConcurrentDictionary<>and aList<>. Rather than making all accesses more expensive in order to make drops less expensive, it makes all reads much cheaper and more scalable, at the expense of making drops more expensive. Since dropping from the cache means we're already paying the expensive cost of creating/parsing/compiling/etc. a new Regex instance, this is a better trade-off, especially since any frequent dropping suggests the consuming app or library needs to revisit its Regex strategy, either using Regex.CacheSize to increase the cache size appropriately, or doing its own caching (e.g. creating the Regex instance it needs and storing it into a field for all future use).The new scheme uses a
ConcurrentDictionary<Key,Node>, aList<Node>, and a fast-path field storing the most recently used Regex instance (just as the existing implementation did). On lookups, if the fast-path field has the matching value, it's just returned. Otherwise, the dictionary is consulted, and if the item is found, the fast-path field is updated. No locking at all is employed, and only a few volatile read/writes are used to update a "last access stamp" that's used to indicate importance if/when items do need to be expunged. On additions, we do still take a global lock and add to the cache. If this puts us over our cache size, we pick an item from the list and remove it. If the list is small, we just examine all of the items looking for the oldest. If the list is larger, we examine a random subset of it; we may not get rid of the absolute oldest item, but it'll be old enough.cc: @danmosemsft, @eerhardt, @ViktorHofer
Results from running master (old) vs this PR (new) on the RegexCache* tests from the dotnet/performance repo: