[Enhancement] Use dynamic batch size for simdjson to parse multiple json document #53056
+457
−22
Conversation
This file contains 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
| return Status::OK(); | ||
| } | ||
| return Status::EndOfFile("all documents of the stream are iterated"); | ||
| return _get_current_impl(row); | ||
| } catch (simdjson::simdjson_error& e) { | ||
| std::string err_msg; | ||
| if (e.error() == simdjson::CAPACITY) { |
There was a problem hiding this comment.
The most risky bug in this code is:
If _batch_size is initially larger than len, the assignment to _doc_stream can result in iterate_many() being called with an invalid length, causing undefined behavior or a crash.
You can modify the code like this:
- _doc_stream = _parser->iterate_many(data, len, len);
+ _doc_stream = _parser->iterate_many(data, len, std::min(_batch_size, len));bf7cb20
to
b403633
Compare
kevincai
reviewed
Nov 25, 2024
be/src/exec/json_parser.cpp
Outdated
| if (_doc_stream_itr != _doc_stream.end()) { | ||
| simdjson::ondemand::document_reference doc = *_doc_stream_itr; | ||
| // simdjson version 3.9.4 and JsonFunctions::to_json_string may crash when json is invalid. | ||
| // https://github.com/StarRocks/StarRocksTest/issues/8327 |
There was a problem hiding this comment.
this link is not accessible from public, remove it.
be/src/exec/json_parser.h
Outdated
| // _last_begin_offsets represent begin offet of last success object in _doc_stream | ||
| size_t _last_begin_offsets = 0; | ||
| // _batch_size using in batch mode parsing | ||
| size_t _batch_size = (config::json_parse_many_batch_size > 0) ? |
There was a problem hiding this comment.
shall put this non-trivial initialization in constructor.
be/src/exec/json_parser.cpp
Outdated
| throw; | ||
| } | ||
|
|
||
| _batch_size = std::min(_batch_size * 2, _len); |
There was a problem hiding this comment.
this batch_size can be changed again and again, should forbid the parser from being used to parse multiple docs, the _batch_size determined by the previous document may not be suitable for next doc.
or you have the way to reset the batch_size before parsing the next doc.
…son document Signed-off-by: srlch <[email protected]>
be49e7d
to
5b42aaf
Compare
Signed-off-by: srlch <[email protected]>
Signed-off-by: srlch <[email protected]>
Signed-off-by: srlch <[email protected]>
Signed-off-by: srlch <[email protected]>
Signed-off-by: srlch <[email protected]>
kevincai
previously approved these changes
Nov 25, 2024
Signed-off-by: srlch <[email protected]>
Signed-off-by: srlch <[email protected]>
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[BE Incremental Coverage Report]✅ pass : 42 / 45 (93.33%) file detail
|
wyb
reviewed
Nov 25, 2024
| Status JsonDocumentStreamParser::parse(char* data, size_t len, size_t allocated) noexcept { | ||
| try { | ||
| _data = data; | ||
| _len = len; | ||
|
|
||
| _doc_stream = _parser->iterate_many(data, len, len); | ||
| _doc_stream = _parser->iterate_many(data, len, | ||
| config::enable_dynamic_batch_size_for_json_parse_many ? _batch_size : _len); |
There was a problem hiding this comment.
_len may be smaller than _batch_size.
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.
Why I'm doing:
In current implementation, JsonDocumentStreamParser use
simdjson::ondemand::parser::iterate_manytoparse multiple JSON document. This API need caller pass the max size of JSON document in the file, says
max_json_lenght_in_filein a given file to allocate the a memory chunk to finish the parsing process.But the problem is that, the caller pass the file size instead of
max_json_lenght_in_fileand allocatehuge memory chunk (which may not be used) almost 5~6 time of the file size. This is a huge memory amplification
What I'm doing:
Introduce
json_parse_many_batch_sizeto control the batch_size passed intosimdjson::ondemand::parser::iterate_many.If
json_parse_many_batch_size > simdjson::dom::MINIMAL_BATCH_SIZE, usejson_parse_many_batch_sizeas batch size,otherwise use
simdjson::dom::DEFAULT_BATCH_SIZE.But simdjson does not guarantee the behavior if the parsing failed because of the small size of batch_size. Any kind of
error/expcetion and even iterator failure (error == EMPTY) can happen in this case. To use to dynamic batch_size, we should
handle all possible error and retry using larger batch_size until the batch_size hit the limit(file size - last success postition)
Fixes #issue
https://github.com/StarRocks/StarRocksTest/issues/8636
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: