Skip to content

Introducing concurrency mode in JsonStream. - #373

Merged
piotte13 merged 6 commits into
masterfrom
jsonstream
Nov 21, 2019
Merged

piotte13 merged 6 commits into
masterfrom
jsonstream

Conversation

@piotte13

Copy link
Copy Markdown
Collaborator

Introducing concurrency mode in JsonStream.

Performance gains are substantial while not adding too much complexity to the current source code. Although I strongly recommend using this new threaded version, the previous non-threaded version is still supported. The JsonStream API remains the same whether you are using the threaded version or not. The only difference for the end-user is the need to add -pthread flag to the compiler in order to enable the threaded version. In the case where the user would not specify the -pthread flag, the non-threaded version will be used. Note that threads are enabled by default with Visual C++ (MSVC).

Here is a chart comparing the speed of the different alternatives to parse a multiline JSON:

Chart.png

How it works

Context: The parsing in simdjson is divided into 2 stages. First, in stage 1, we parse the document and find all the structural indexes ({, }, ], [, ,, ", ...) and validate UTF8. Then, in stage 2, we go through the document again and build the tape using structural indexes found during stage 1. Although stage 1 finds the structural indexes, it has no knowledge of the structure of the document nor does it know whether it parsed a valid document, multiple documents, or even if the document is complete.

We found a pretty cool algorithm that allows us to quickly identify the position of the last JSON document in a given batch. Knowing exactly where the end of the batch is, we no longer need for stage 2 to finish in order to load a new batch. We already know where to start the next batch. Therefore, we can run stage 1 on the next batch concurrently while the main thread is going through stage 2. Now, running stage 1 in a different thread can, in best cases, remove almost entirely it's cost and replaces it by the overhead of a thread, which is orders of magnitude cheaper. Ain't that awesome!

Future Development

This pull request also acts as a proof of concept for future development where the standard parsing (single file) could be threaded for substantial gains. This technique could be implemented by modifying the stage 2 so that it could parse in streaming mode, while stage 1 would run concurrently. What I mean here by "streaming mode" is that instead of stage 2 parsing a file all at once, it could parse it piece by piece, similar to the JsonStream implementation.
Therefore, running stage 1 concurrently would reduce its cost substantially, which generally represents ~50% of the total work.

TL/DR: We could potentially make simdjson as much as ~50% faster in some cases. Bigger files would get bigger gains.

#188 #274

@lemire

lemire commented Nov 20, 2019

Copy link
Copy Markdown
Member

@piotte13 Something is wrong with the PR from the point of view of git. I think you need to correct your commits: the PR confuses git and GitHub.

@piotte13

Copy link
Copy Markdown
Collaborator Author

@lemire Yes, I did not expect the PR to include the previous PR from this branch... I will fix this.

@piotte13

Copy link
Copy Markdown
Collaborator Author

@pauldreik I get the following error with the new fuzzer. Is that a bug? Not sure I understand why the build is not passing here.

image

@pauldreik

Copy link
Copy Markdown
Member

Hmm, this thing happened to me before, on another project. That time, it was because of a poor seed corpus. In this case, the initial corpus is really good, so I am surprised to see it. I reran the fuzz job for you, but it did not help. I will have to checkout your branch and see if I can reproduce it. Unfortunately I might not be able to until friday. Please don't merge this pull request until I have had a chance to look at it, it will break oss-fuzz.
In the meanwhile, you can build the fuzzer yourself and try to bisect the problem. That would be helpful. The corpus is available without authentication. Checkout the action file.

@pauldreik

Copy link
Copy Markdown
Member

I ran git bisect on this. The first bad commit on the jsonstream branch is the first commit: e31c673

The reason is the pthreads flag set in cmake. I made a comment on it: e31c673#r36059274

Removing that line makes the fuzzers work, please try and push and we will see if the CI job goes happy again!

@piotte13

Copy link
Copy Markdown
Collaborator Author

But I guess you might be recommending the following...

set(CMAKE_THREAD_PREFER_PTHREAD TRUE)
set(THREADS_PREFER_PTHREAD_FLAG TRUE)
find_package(Threads)

This does not seem to work in our case. I will remove -pthread flag from cmake for now, see if it is indeed the problem.

@lemire

lemire commented Nov 21, 2019

Copy link
Copy Markdown
Member

This does not seem to work in our case.

Can you elaborate?

@piotte13 piotte13 closed this Nov 21, 2019
@piotte13 piotte13 reopened this Nov 21, 2019
@lemire

lemire commented Nov 21, 2019

Copy link
Copy Markdown
Member

I think we want cmake to create a threaded build. Can we check whether it is indeed the case without additional flags?

@piotte13

Copy link
Copy Markdown
Collaborator Author

@lemire I think what I should rather have said is that I couldn't get it to work yet. It might be my limited knowledge with cmake, but when I tried this technique, it was not enabling the threading, at least the _REENTRANT flag was not defined...

Still working on it...

@piotte13

piotte13 commented Nov 21, 2019

Copy link
Copy Markdown
Collaborator Author

Seems like this last commit solves the issue!
In addition to

set(CMAKE_THREAD_PREFER_PTHREAD TRUE)
set(THREADS_PREFER_PTHREAD_FLAG TRUE)
find_package(Threads)
...
add_executable(jsonstream_test jsonstream_test.cpp)
target_link_libraries(jsonstream_test Threads::Threads)

We need to add the following if statement. It will enable _REENTRANT when needed.

if(CMAKE_USE_PTHREADS_INIT)
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -pthread")
endif()

@piotte13
piotte13 merged commit 29fc515 into master Nov 21, 2019
@jkeiser
jkeiser deleted the jsonstream branch December 24, 2019 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants