Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Available allocator choices for RapidJSON are problematic in multi-threaded context #241

Open
Selmar opened this issue Jan 28, 2020 · 5 comments

Comments

@Selmar
Copy link
Contributor

Selmar commented Jan 28, 2020

In a multi-threaded context, I still want to be able to use the memory pool allocator, but the only options I have are using a static document or using the CRT allocator. However, the latter causes a lot of contention on allocations when parsing multiple files in parallel.

At first glance, I think making this possible requires passing the json document through in the serialization functions to be able to get the allocator, which if why I've opened an issue instead of simply creating a pull request.

@syoyo
Copy link
Owner

syoyo commented Jan 28, 2020

However, the latter causes a lot of contention on allocations when parsing multiple files in parallel.

Do you mean you face an issue when parsing multiple glTF files by instanciating TinyGLTF class for each glTF file in multi-thread context?

Probably we may need to prepare different json context for each parsing and serialization

@Selmar
Copy link
Contributor Author

Selmar commented Jan 28, 2020

Do you mean you face an issue when parsing multiple glTF files by instanciating TinyGLTF class for each glTF file in multi-thread context?

Yes.

Probably we may need to prepare different json context for each parsing and serialization

Agreed. Since the serialization is currently done with static functions, this requires a bit of refactoring.

In fact, due to the usage of std:: types in the tinygltf code itself, there is also contention on memory allocation after the json parsing, but changing this is likely a significant API change.

@syoyo
Copy link
Owner

syoyo commented Jan 28, 2020

Do you mean you face an issue when parsing multiple glTF files by instanciating TinyGLTF class for each glTF file in multi-thread context?

Yes.

I see. For parsing, I am considering to use sajson https://github.com/chadaustin/sajson .

Its simple, cross-platform and should have enough features for parsing glTF JSON. Good thing is it has single static memory allocation feature for parsing(dynamic allocation is also provided). This feature will be helpful when parsing multiple JSON files with multi-threading.

According to his blog https://chadaustin.me/tag/sajson/ , memory allocation for JSON AST construction can be bounded: sizeof(int64) * N, where N is the number of characters(bytes) in JSON data. Most of glTF JSON are small, so single allocation should work well.

@Selmar
Copy link
Contributor Author

Selmar commented Jan 28, 2020

RapidJSON works absolutely fine for us, it's just a poor default choice of allocator in tinygltf. A few allocations are really not a problem; it only becomes problematic when every json object requires a separate allocation (like std::string).

Looking at some benchmarks here, I don't see any reason to change from RapidJSON to anything else.

@Skylion007
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants