[dbnode] Add index regexp DFA compilation cache to avoid allocating DFAs for same expressions#2814
Merged
Conversation
arnikola
reviewed
Oct 28, 2020
arnikola
reviewed
Oct 28, 2020
arnikola
reviewed
Oct 28, 2020
| cacheErrors: opts.CacheErrorsByDefault, | ||
| concurrencyLeases: concurrencyLeases, | ||
| metrics: &lruCacheMetrics{ | ||
| entries: tallyScope.Gauge(entriesGauge), |
Collaborator
There was a problem hiding this comment.
nit: may be better to have tallyScope be scoped to cache rather than requiring one that's scoped to be passed in
Collaborator
Author
There was a problem hiding this comment.
Yeah true, done 👍
…:m3db/m3 into r/add-index-regexp-dfa-compilation-cache
arnikola
reviewed
Oct 28, 2020
Comment on lines
+466
to
+472
| select { | ||
| case <-ctx.Done(): | ||
| return c.cacheLoadComplete(key, time.Time{}, nil, UncachedError{ctx.Err()}) | ||
| case <-c.concurrencyLeases: | ||
| } | ||
|
|
||
| defer func() { c.concurrencyLeases <- struct{}{} }() |
Collaborator
There was a problem hiding this comment.
nit; maybe slightly safer as:
Suggested change
| select { | |
| case <-ctx.Done(): | |
| return c.cacheLoadComplete(key, time.Time{}, nil, UncachedError{ctx.Err()}) | |
| case <-c.concurrencyLeases: | |
| } | |
| defer func() { c.concurrencyLeases <- struct{}{} }() | |
| select { | |
| case <-ctx.Done(): | |
| return c.cacheLoadComplete(key, time.Time{}, nil, UncachedError{ctx.Err()}) | |
| case <-c.concurrencyLeases: | |
| defer func() { c.concurrencyLeases <- struct{}{} }() | |
| } | |
arnikola
reviewed
Oct 28, 2020
Comment on lines
+76
to
+83
| // As returns true if the caller is asking for the error as an uncached error | ||
| func (e UncachedError) As(target interface{}) bool { | ||
| if uncached, ok := target.(*UncachedError); ok { | ||
| uncached.Err = e.Err | ||
| return true | ||
| } | ||
| return false | ||
| } |
Collaborator
There was a problem hiding this comment.
nit; dont think we use these
arnikola
reviewed
Oct 28, 2020
Comment on lines
+102
to
+109
| // As returns true if the caller is asking for the error as an cached error | ||
| func (e CachedError) As(target interface{}) bool { | ||
| if cached, ok := target.(*CachedError); ok { | ||
| cached.Err = e.Err | ||
| return true | ||
| } | ||
| return false | ||
| } |
Collaborator
There was a problem hiding this comment.
nit; dont think we use these
arnikola
reviewed
Oct 28, 2020
| } | ||
|
|
||
| // If the entry exists and has not expired, it's a hit - return it to the caller | ||
| if exists && entry.expiresAt.After(c.now()) { |
Collaborator
There was a problem hiding this comment.
Does this need to check that it's not expired? You'll likely be regenerating the same entry anyway here
arnikola
reviewed
Oct 28, 2020
| } | ||
|
|
||
| // There is an entry and it's valid, return it. | ||
| return value, nil |
Collaborator
There was a problem hiding this comment.
Do we want this to update the entry TTL?
arnikola
reviewed
Oct 28, 2020
| @@ -0,0 +1,47 @@ | |||
| // Copyright (c) 2020 Uber Technologies, Inc. | |||
Collaborator
There was a problem hiding this comment.
nit; update filename too
soundvibe
pushed a commit
that referenced
this pull request
Oct 29, 2020
* master: Support dynamic namespace resolution for embedded coordinators (#2815) [dbnode] Add index regexp DFA compilation cache to avoid allocating DFAs for same expressions (#2814) [dbnode] Set default cache on retrieve to false, prepare testing with cache none (#2813) [tools] Output annotations as base64 (#2743) Add Reset Transformation (#2794) [large-tiles] Fix for a reverse index when querying downsampled namespace (#2808) [dbnode] Evict info files cache before index bootstrap in peers bootstrapper (#2802) [dbnode] Bump default filesystem persist rate limit (#2806) [coordinator] Add metrics and configurability of downsample and ingest writer worker pool (#2797) [dbnode] Fix concurrency granularity of seekerManager.UpdateOpenLease (#2790) [dbnode] Use correct import path for atomic library (#2801) Regenerate carbon automapping rules on namespaces changes (#2793) Enforce matching retention and index block size (#2783) [query] Add ClusterNamespacesWatcher and use to generate downsample automapper rules (#2782) [query][dbnode] Wire up etcd-backed Clusters implementation in coordinator (#2758) [query] Fix quantile() argument not being passed through (#2780) # Conflicts: # src/query/parser/promql/matchers.go
soundvibe
pushed a commit
that referenced
this pull request
Oct 29, 2020
* master: [query] Improve precision for variance and stddev of equal values (#2799) Support dynamic namespace resolution for embedded coordinators (#2815) [dbnode] Add index regexp DFA compilation cache to avoid allocating DFAs for same expressions (#2814) [dbnode] Set default cache on retrieve to false, prepare testing with cache none (#2813) [tools] Output annotations as base64 (#2743) Add Reset Transformation (#2794) [large-tiles] Fix for a reverse index when querying downsampled namespace (#2808) [dbnode] Evict info files cache before index bootstrap in peers bootstrapper (#2802) [dbnode] Bump default filesystem persist rate limit (#2806) [coordinator] Add metrics and configurability of downsample and ingest writer worker pool (#2797) [dbnode] Fix concurrency granularity of seekerManager.UpdateOpenLease (#2790) [dbnode] Use correct import path for atomic library (#2801) Regenerate carbon automapping rules on namespaces changes (#2793) Enforce matching retention and index block size (#2783) [query] Add ClusterNamespacesWatcher and use to generate downsample automapper rules (#2782) [query][dbnode] Wire up etcd-backed Clusters implementation in coordinator (#2758) [query] Fix quantile() argument not being passed through (#2780) [query] Add "median" aggregation to Graphite aggregate() function (#2774) [r2] Ensure KeepOriginal is propagated when adding/reviving rules (#2796) [dbnode] Update default db read limits (#2784)
rallen090
pushed a commit
that referenced
this pull request
Nov 4, 2020
…FAs for same expressions (#2814)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
This reduces allocations building regexp DFA for the same regexp queries one each index query, instead using an LRU to keep the most frequently queried regexp DFA's around for reuse. Also includes metrics.
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: