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

Read Submission should bypass OSE Threads #1791

Closed
nicmichael opened this issue Nov 6, 2018 · 3 comments
Closed

Read Submission should bypass OSE Threads #1791

nicmichael opened this issue Nov 6, 2018 · 3 comments

Comments

@nicmichael
Copy link
Contributor

BUG REPORT

  1. Please describe the issue you observed:

Profiling of our Bookkeeper Client code for read requests shows that client threads spend half of their time in dispatching requests to OrderedExecutors (just the dispatch itself, not the execution inside OSE): 54% of their CPU time is spent in OrderedExecutor.executeOrdered() (called by LedgerHandle.readEntriesInternalAsync()). The high time spend in request submission to OSE is largely caused by Linux scheduling cost, that is the cost of dispatching the OSE thread to CPU: 42% of total time (3/4th of executeOrdered() time), threads spend in Unsafe.unpark(), which is essentially Linux scheduling/dispatching of another thread.

It seems unnecessary for all reads to have to go through OrderedExecutors. Reads that do not require ordering (reads to read-only ledger handles?) should bypass OrderedExecutors, and read submission instead be executed directly in the context of the client thread without any context-switching to another thread (meaning, the client thread dispatches the read to Netty). This should both reduce overall CPU consumption of Bookkeeper client code, and reduce latency of reads by eliminating one context-switch and serialization/queuing of reads on OSEs.

Experiments with a prototype of reads to read-only ledger handles have shown significant improvements in both overall CPU consumption as well as read latency. The additional work client threads have to do (the dispatch of the read requests to netty) is roughly the same as the (saved) dispatch cost to OSE, so the change turns out to be neutral for CPU consumption of client threads. In some experiments, the savings even exceed the additional work, and client threads consume less cpu even though they "do more". It also frees up lots of resources in OSE threads. Since it eliminates one context-switch in read submission and also avoids serialization of reads to the same ledger (or ledgers hashing to the same OSE), it also reduces read latency. For a mixed read-write workload (14,000 reads/sec on read-only ledgers, 4,000 writes/sec on another set of ledgers), this change has reduced CPU consumption of OSE threads by 25%, kept CPU consumption of client (and Netty) threads the same, and yielded a 6% improvement of read latency (as measured by BK Client).

@nicmichael
Copy link
Contributor Author

profiling_executeordered

@merlimat
Copy link
Contributor

merlimat commented Nov 6, 2018

I think pegging requests to thread should avoid the mutex contention.
Eg: reading multiple entries from different bookies, when these requests are completed we check if the all entries for the overall range was completed. This is protected by a mutex and if we have multiple thread it will lead to lower throughput

@nicmichael
Copy link
Contributor Author

@merlimat I'm not sure I understand your comment. Are you referring to the completion of the read request?

I was thinking of only changing the read submission, but not the completion. Let me quickly open a pull request so you can see what changes I am proposing.

nicmichael added a commit to nicmichael/bookkeeper that referenced this issue Nov 6, 2018
This change executes read submissions (PendingReadOp) on read-only ledger handles directly inside the client thread instead of submitting them to Ordered Executors.

Tests with a prototype have shown significant improvements in both overall CPU consumption as well as read latency. The additional work client threads have to do (the dispatch of the read requests to netty) is roughly the same as the (saved) dispatch cost to OSE, so the change turns out to be neutral for CPU consumption of client threads. In some experiments, the savings even exceed the additional work, and client threads consume less cpu even though they "do more". It also frees up lots of resources in OSE threads. Since it eliminates one context-switch in read submission and also avoids serialization of reads to the same ledger (or ledgers hashing to the same OSE), it also reduces read latency. For a mixed read-write workload (14,000 reads/sec on read-only ledgers, 4,000 writes/sec on another set of ledgers), this change has reduced CPU consumption of OSE threads by 25%, kept CPU consumption of client (and Netty) threads the same, and yielded a 6% improvement of read latency (as measured by BK Client).
@sijie sijie closed this as completed in 6b99ff7 Nov 8, 2018
sijie pushed a commit that referenced this issue Nov 8, 2018
Profiling of our Bookkeeper Client code for read requests shows that client threads spend half of their time in dispatching requests to OrderedExecutors (just the dispatch itself, not the execution inside OSE): 54% of their CPU time is spent in OrderedExecutor.executeOrdered() (called by LedgerHandle.readEntriesInternalAsync()). The high time spend in request submission to OSE is largely caused by Linux scheduling cost, that is the cost of dispatching the OSE thread to CPU: 42% of total time (3/4th of executeOrdered() time), threads spend in Unsafe.unpark(), which is essentially Linux scheduling/dispatching of another thread.

This change executes read submissions (PendingReadOp) on read-only ledger handles directly inside the client thread instead of submitting them to Ordered Executors.

Tests with a prototype have shown significant improvements in both overall CPU consumption as well as read latency. The additional work client threads have to do (the dispatch of the read requests to netty) is roughly the same as the (saved) dispatch cost to OSE, so the change turns out to be neutral for CPU consumption of client threads. In some experiments, the savings even exceed the additional work, and client threads consume less cpu even though they "do more". It also frees up lots of resources in OSE threads. Since it eliminates one context-switch in read submission and also avoids serialization of reads to the same ledger (or ledgers hashing to the same OSE), it also reduces read latency. For a mixed read-write workload (14,000 reads/sec on read-only ledgers, 4,000 writes/sec on another set of ledgers), this change has reduced CPU consumption of OSE threads by 25%, kept CPU consumption of client (and Netty) threads the same, and yielded a 6% improvement of read latency (as measured by BK Client).

Master Issue: #1791: Read Submission should bypass OSE Threads

Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Andrey Yegorov <None>, Sijie Guo <sijie@apache.org>, Matteo Merli <mmerli@apache.org>

This closes #1792 from nicmichael/DirectRead, closes #1791
sijie added a commit to sijie/bookkeeper that referenced this issue Nov 10, 2018
*Motivation*

when apache#1791 is cherry-picked into branch-4.8, it breaks the build.

*Changes*

Fix the compilation issue.
sijie added a commit to sijie/bookkeeper that referenced this issue Nov 10, 2018
*Motivation*

apache#1791 improves performance by bypassing OSE threads for read request submission.
however the change doesn't handle `tailing` ReadOnlyLedgerHandle

*Changes*

ReadOnlyLedgerHandle overrides `isHandleWritable` and return `false` always.
eolivelli pushed a commit that referenced this issue Nov 13, 2018

Descriptions of the changes in this PR:

*Motivation*

when #1791 is cherry-picked into branch-4.8, it breaks the build.

*Changes*

Fix the compilation issue.

Related Issue: #1791 




Reviewers: Enrico Olivelli <eolivelli@gmail.com>

This closes #1802 from sijie/fix_branch48, closes #1791
eolivelli added a commit to eolivelli/bookkeeper that referenced this issue Mar 14, 2022
nicoloboschi added a commit to datastax/bookkeeper that referenced this issue Mar 14, 2022
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

No branches or pull requests

2 participants