Parallel image parsing for low latency use cases #470
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
… and passes index to callback, adds ParseStringArrayProperty and uses it for extension parsing
…son array into a vector<T> This makes ParseNumberArrayProperty, ParseIntegerArrayProperty, and ParseStringArrayProperty just special cases of the base function. ParseObjectArrayProperty is also a special case which replaces ForEachObjInArray. ParseArrayProperty also resizes the vector to the json array's size before iteration, preventing any extra allocations. As a side effect, this gives pointer stability during the parsing operation.
|
If the parsing speed is dominated by image decoding, I think preferred approach would be using delayed decoding strategy. This will keep code change small. PR of having delayed(threaded) decoding of images by default would be nice. For refactoring |
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.
Briefly: I developed a gltf parser for my needs but want to switch to a 3rd party solution. Knew tinygltf was an option. Did some testing and it was slow. Wanted to apply the multithreading strategy I used on my parser.
I know that sophisticated projects have their thread managers and can do a better job. The case I optimized for is small apps with the sole purpose of displaying a single model as fast as possible, such as Windows 3D Viewer or gltf-viewers. Or any other case where the latency is important and there are idle cores. I observed x4 or so speedup with gltf files with 40MB+ image files.
The initial implementation with minimal changes I did looked like a hack, so I had to make larger changes. While reading the code, I realized some repetition and made a generic function,
ParseArrayProperty, that unified many use cases (seeLoadFromString). This function also resizes the vector before any insertion, which prevents any unnecessary allocations.There is an issue though, I don't get the
requiredargument ofParse*Propertyfunctions. It is just a flag that prevents error logging, while the name suggests something that should affect the program flow. Like, if that property is absent, parsing should stop.LoadFromStringhas such a section that some members are checked and if any are absent, returns false. Also, error logging can be suppressed by passing a nullptr to err as it is already checked.I implemented the
ParseArrayPropertywith a different behaviour, which worked for LoadFromString but failed for others. Hence the draft pull request. Do you have any suggestions on what to do? Should I change the argument's name or behaviour?Also, I measured the time it takes to copy all the pixels from stb_image buffer to a vector, which takes quite a big portion of the parsing process. I think capturing it with a std::unique_ptr can totally eliminate that.