[dbnode] Remove tag decoders from write tagged requests. #3472
Conversation
Introduced TagMetadataResolver which wraps convertion methods from encoded tags, tag iterators and decoded tags.
Codecov Report
@@ Coverage Diff @@
## master #3472 +/- ##
========================================
- Coverage 56.0% 55.9% -0.2%
========================================
Files 548 549 +1
Lines 61188 61146 -42
========================================
- Hits 34308 34197 -111
- Misses 23797 23857 +60
- Partials 3083 3092 +9
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
| // namespace | ||
| pooledIDs := make([]writeBatchPooledReqID, 1+(2*client.DefaultWriteBatchSize)) | ||
| // NB(r): Make pooled IDs 1x the default write batch size plus an extra one for the namespace | ||
| pooledIDs := make([]writeBatchPooledReqID, 1+client.DefaultWriteBatchSize) |
There was a problem hiding this comment.
Since we don't pool tag decoders, we can reduce pooledIDs by 2x.
| @@ -108,15 +108,14 @@ func (b *writeBatch) Add( | |||
| func (b *writeBatch) AddTagged( | |||
| originalIndex int, | |||
| id ident.ID, | |||
| tagIter ident.TagIterator, | |||
There was a problem hiding this comment.
No need for TagIterator when batching writes.
src/x/ident/types.go
Outdated
| type TagMetadataResolver interface { | ||
| // Resolve resolves doc metadata. | ||
| Resolve(id ID) (doc.Metadata, error) | ||
| } |
There was a problem hiding this comment.
The one thing that could reduce allocations even further would be to make this a struct instead of an interface type (i.e. type TagMetadataResolver struct) and then just have special constructors for each type and based on which constructor was used set some type of boolean or enum on the struct so that when Resolve(id) (doc.Metadata, error) is called it knows what to do.
For an example see the following: NewMetadataFromIDAndTagIterator and NewMetadataFromIDAndTags and then see the ResetOrReturnProvidedTagIterator method.
That way it can be passed on the stack and doesn't need to be heap allocated (which is required with an interface).
| // SetTagDecoderPool sets the tag encoder pool. | ||
| SetTagDecoderPool(value serialize.TagDecoderPool) Options | ||
|
|
||
| // TagDecoderPool returns the tag encoder pool. | ||
| TagDecoderPool() serialize.TagDecoderPool |
There was a problem hiding this comment.
Great to see all this cleanup happening!
Moved TagMetadataResolver to convert namespace. Other small changes after review.
* master: [query] Add Graphite movingWindow() function (#3484) [lint] Add missing build tags to linter configuration (#3480) [query] Fix Graphite nil arg not interpreted as explicit nil (#3481) [query] Fix Graphite context time window expansions not being cumulative [readme] Remove coverage badge due to CodeCov not able to execute (#3476) [query] Fix using median with aggregateWithWildcards and support more aggregate functions (#3469) [matcher] [coordinator] Add RequireNamespaceWatchOnInit option (#3468) [buildkite] Remove codecov uploading from unit tests (#3475) [aggregator] Add integration test for aggregator placement changes (#3465) [tools] Use streaming reads in read_data_files (#3474) [tools] Close the reader in read_data_files (#3473)
What this PR does / why we need it:
Removes tag decoders and their pools from write paths. This change should reduce memory usage and avoid huge memory usage spikes when writes are increased.
Introduced
TagMetadataResolverwhich wraps conversion methods from encoded tags, tag iterators and decoded tags.Heap profile before this PR:
Heap profile with the changes from this PR:
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?: