syoyo / tinygltf

Header only C++11 tiny glTF 2.0 library
MIT License
2.03k stars 410 forks source link

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

Open Selmar opened 4 years ago

Selmar commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

@Selmar @syoyo Also see these benchmarks: https://github.com/simdjson/simdjson#performance-results