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

RoNode: read-only parallelization with minimal overhead #53

Merged
merged 6 commits into from
Apr 14, 2019

Conversation

dginev
Copy link
Member

@dginev dginev commented Apr 13, 2019

A simple and pragmatic take on a multi-threaded primitive (see #47 ).

A benchmark on a huge 112 MB XML file with this PR reveals:

single thread DFS count
  time:   [1.1032 s 1.1079 s 1.1120 s]                                     

single thread DFS count+length
  time:   [1.3465 s 1.3498 s 1.3532 s]

read-only single thread DFS count
  time:   [302.88 ms 303.53 ms 304.31 ms]

read-only single thread DFS count+length
  time:   [534.57 ms 535.03 ms 535.71 ms]

read-only multi thread DFS count
  time:   [33.617 ms 34.120 ms 34.625 ms]

read-only multi thread DFS count+length
  time:   [51.574 ms 52.269 ms 52.765 ms]

Where the single thread uses the Node implementation of master, while the read-only and multi thread benchmarks use the new RoNode construct.

The pragmatics of the PR were:

  • only solve parallel processing for one use case (= my current need), which keeps it simple and easy to understand
  • only pay for features you use mantra -- my use case is entirely one of "scanning" an existing document, so no mutation, no bookkeeping, no data races.
  • As libxml eagerly loads the entire document in memory, it is safe to have concurrent read operations executing on the tree. Conversely, it is very unsafe to allow any mutations in parallel without first adding a very expensive layer of bookkeeping and mutual exclusion.
  • The PR is a conservative extension over master, which is unchanged, it only adds a new RoNode primitive and related logic for using it.
  • RoNode is just Node with all mutation and bookkeeping removed, including related methods. A bit of boilerplate overload to copy-paste it all, one could imagine it becoming a NodeReader trait instead.
  • Importantly, Sync and Send are declared safe on RoNode, while they aren't on Node, which uses single-threaded abstractions.

As you can see from the sample benchmark, on a huge file with a very minimal compute job you get a 10x speedup when running using rayon parallel iterators on my 32 logical thread CPU.

@dginev
Copy link
Member Author

dginev commented Apr 13, 2019

I would still want to add some tests here, and test on my actual application, before suggesting a merge, but welcoming feedback for this simplicity-seeking approach cc @triptec

@dginev
Copy link
Member Author

dginev commented Apr 14, 2019

I have updated the benchmark and feel it's even more impressive to compare the master single-threaded performance to the read-only multi-threaded one -- 1.3s to 51ms, a twenty-five fold improvement in speed.

@triptec
Copy link
Collaborator

triptec commented Apr 14, 2019

Sounds good to me

@dginev
Copy link
Member Author

dginev commented Apr 14, 2019

Thanks @triptec ! All existing code will remain unaffected, so feeling good about releasing the PR today, just pressing the buttons 👍

@dginev dginev merged commit 34ba835 into master Apr 14, 2019
@dginev
Copy link
Member Author

dginev commented Apr 14, 2019

Shipped as 0.2.10 🎉

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.

None yet

2 participants