[ML] Clearing JSON memory allocators #30
Merged
Conversation
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
Use scoped JSON memory allocators to avoid excessive memory accumulation where-ever the JSON output writer is persisted. Fixes #26
tveasey
reviewed
Apr 3, 2018
lib/api/CJsonOutputWriter.cc
Outdated
| @@ -430,6 +431,10 @@ const CJsonOutputWriter::TStrVec &CJsonOutputWriter::fieldNames(void) const | |||
| bool CJsonOutputWriter::writeRow(const TStrStrUMap &dataRowFields, | |||
| const TStrStrUMap &overrideDataRowFields) | |||
| { | |||
| typedef core::CScopedRapidJsonPoolAllocator<core::CRapidJsonConcurrentLineWriter> TScopedAllocator; | |||
There was a problem hiding this comment.
As per #32, can we switch to using TScopedAllocator = ... here and throughout.
|
Otherwise, this is LGTM. The two questions I have are:
|
Replacing typedef declarations with 'using' instead.
|
Thanks for the review comments @tveasey. The extra Json documents in use in CForecastDataSink came in to existence when I refactored the Json output writer and memory allocator. The scoped memory allocator is cached, it does not actually get destroyed at end of scope. Rather, it simply calls |
tveasey
approved these changes
Apr 3, 2018
Added comments to CRapidJsonWriterBase to clarify the intent and purpose of the allocator stack and the interaction with CScopedRapidJsonPoolAllocator
droberts195
reviewed
Apr 4, 2018
include/core/CRapidJsonWriterBase.h
Outdated
| // for the writer at nested scope. | ||
| // | ||
| // Note that allocators are not destroyed by the pop operation, they persist for the lifetime of the | ||
| // writer in a cache for swift retrieval. |
There was a problem hiding this comment.
You could use //! comments so that this is included in the Doxygen output.
tveasey
reviewed
Apr 4, 2018
include/core/CRapidJsonWriterBase.h
Outdated
| @@ -100,6 +100,18 @@ class CRapidJsonWriterBase : public JSON_WRITER<OUTPUT_STREAM, SOURCE_ENCODING, | |||
| public: | |||
| using TRapidJsonWriterBase = JSON_WRITER<OUTPUT_STREAM, SOURCE_ENCODING, TARGET_ENCODING, STACK_ALLOCATOR, WRITE_FLAGS>; | |||
|
|
|||
| // Instances of this class may very well be long lived, potentially for the lifetime of the application. | |||
| // Over the course of that lifetimme resources will accumulate in the underlying rapidjson memory | |||
|
I tested it and can not reproduce #26 with this fix. |
edsavage
added a commit
that referenced
this pull request
Apr 5, 2018
Use scoped JSON memory allocators to avoid excessive memory accumulation where-ever the JSON output writer is persisted. Fixes #26
droberts195
pushed a commit
that referenced
this pull request
Apr 23, 2018
Use scoped JSON memory allocators to avoid excessive memory accumulation where-ever the JSON output writer is persisted. Fixes #26
droberts195
pushed a commit
that referenced
this pull request
Apr 23, 2018
[ML] Clearing JSON memory allocators Use scoped JSON memory allocators to avoid excessive memory accumulation where-ever the JSON output writer is persisted. Fixes #26
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Use scoped JSON memory allocators to avoid excessive memory accumulation
where-ever the JSON output writer is persisted.
Fixes #26