Refactored OrderedSafeExecutor and OrderedScheduler #1309

merlimat · 2018-03-29T19:39:42Z

As outlined in https://lists.apache.org/thread.html/102383ea42f473f36720637e41af0ee83fc38d9f992736e0d1a7f985@%3Cdev.bookkeeper.apache.org%3E

right now OrderedSafeExecutor is implemented on top of OrderedScheduler. There are few problems with this approach that are causing impact on performance:

OrderedScheduler is a ScheduledExecutorService which uses a priority queue for tasks. The priority queue has a single mutex for both publishers/consumers on the queue
There are many objects created for each task submission, due to listenable future decorators

Since in all cases in critical write/read path we don't need delay task execution or futures, we should try to have a light weight execution for that.

Modifications

Inverted the hierarchy between OrderedSafeExecutor and OrderedScheduler. Now the base class is OrderedSafeExecutor and the other extends from it, since it provides additional methods.
Moved OrderedSafeExecutor in bookkeeper-common since OrderedScheduler was already there.
Moved OrderedSafeGenericCallback in its own file, since it needs to be in bookkeeper-server module at this point.
Changed some method names from submitOrdered() into executeOrdered() to be consistent with JDK name (submit() returns a future while execute() returns void).
Changed BookKeeper instance of scheduler into OrderedScheduler so that the few cases which were using the mainWorkerPool could be easily converted to use the scheduler instead.

sijie · 2018-03-29T19:58:11Z

bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedSafeExecutor.java

+ * method.
+ */
+@Slf4j
+public class OrderedSafeExecutor implements ExecutorService {


Since we are in a new package, can you rename it to "OrderedExecutor" to be consistent with "OrderedScheduler"? This also make a distinguish from the old OrderedSafeExecutor.

Yes, makes sense.

Also, regarding SafeRunnable, I think we should be doing that internally, just taking a Runnable, though that's another story from this PR.

eolivelli

Great idea!
I left some questions but overall is very good

eolivelli · 2018-03-29T21:06:43Z

bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/BoundedExecutorService.java

+
+    @Override
+    public <T> List<Future<T>> invokeAll(Collection<? extends Callable<T>> tasks) throws InterruptedException {
+        checkQueue();


Shouldn't you pass something like the size of the list? checkQueue assumes you are going to add 1 item.
I mean something like
checkQueue(tasks.size())

eolivelli · 2018-03-29T21:08:43Z

bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedExecutor.java

+    /**
+     * Abstract builder class to build {@link OrderedScheduler}.
+     */
+    public abstract static class AbstractBuilder<T extends OrderedExecutor> {


Why public?

Yes, it could be package private

Actually, it needs to be public because the builder methods are returning AbstractBuilder<T> so that needs to be visible to callers.

eolivelli · 2018-03-29T21:11:04Z

bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedExecutor.java

+    }
+
+    public <T> ListenableFuture<T> submitOrdered(long orderingKey, Callable<T> task) {
+        SettableFuture<T> future = SettableFuture.create();


This is more lightweight than CompletableFuture or is there any other reason to use this instead of CF?

The existing code for submitting a Callable was already expecting ListenableFuture. We can (and should) change to CompletableFuture though I didn't want to cram too many changes in a single PR.

…ecutor

jvrao · 2018-03-30T01:18:48Z

@dlg99 Please review.

jvrao · 2018-03-30T03:24:42Z

@merlimat As this is perf focused change, any data / micro benchmarks to validate our assumptions?

dlg99 · 2018-03-30T17:52:03Z

bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedExecutor.java

+    /**
+     * A builder class for an OrderedSafeExecutor.
+     */
+    public static class Builder extends AbstractBuilder<OrderedExecutor> {


can we use lombok's @builder to replace this? sounds like one can override build() method: projectlombok/lombok#1144

Sure, I didn't change it here to limit the scope of this already big PR. I think we should do in separate change.

sijie · 2018-04-01T19:38:39Z

@eolivelli can you review this again? @merlimat already addressed your comments.

eolivelli

+1 great work @merlimat !

merlimat · 2018-04-02T17:24:42Z

@jvrao I have added a simple microbenchmark (22eba18)

Results:

Benchmark                                 (executorName)   Mode  Cnt    Score     Error   Units
OrderedExecutorBenchmark.submitAndWait    JDK-ThreadPool  thrpt    3  417.014 ±  54.313  ops/ms
OrderedExecutorBenchmark.submitAndWait   OrderedExecutor  thrpt    3  406.132 ± 260.011  ops/ms
OrderedExecutorBenchmark.submitAndWait  OrderedScheduler  thrpt    3  337.974 ± 325.934  ops/ms

dlg99 · 2018-04-02T17:50:42Z

@merlimat @jvrao "337.974 ± 325.934" is a smell. it's too much of a range.
Results of "JDK-ThreadPool" and "OrderedExecutor" are close except OE's have wide range.
It is either a problem with test or with the change itself. Change looks ok to me so let's assume the test is at fault.

looking at microbenchmark, it always uses 1 threaded executor and 16 threads that submit/wait for tasks. I think it hits bottleneck in that one thread's rather than in the queue passing tasks between threads. I'd create executor with 8 (half or producer's) threads an give it a shot. or run the test with 1/2/4/8/16 threads in executor, use num threads as another Param for the test.

Refactored OrderedSafeExecutor and OrderedScheduler

9268684

merlimat added the type/improvement label Mar 29, 2018

merlimat added this to the 4.7.0 milestone Mar 29, 2018

merlimat self-assigned this Mar 29, 2018

merlimat requested review from ivankelly, jiazhai, jvrao and sijie March 29, 2018 19:39

sijie reviewed Mar 29, 2018

View reviewed changes

Renamed into OrderedExecutor and OrderedGenericCallback

e45f78c

eolivelli requested changes Mar 29, 2018

View reviewed changes

merlimat added 5 commits March 29, 2018 14:47

Correctly handle number of tasks in queue check

957c983

Merge remote-tracking branch 'apache/master' into refactor-ordered-ex…

c5f70d5

…ecutor

Removed "final" modifier on checkQueue

c91259b

Fixed submitOrdered in DL code

7c529dd

Fixed class names in integration tests

5d68d43

Fixed array initialization type

b8175f3

merlimat added 3 commits March 29, 2018 21:24

More fixes for submitOrdered

019d520

Fix for TestReadAheadEntryReader

54a72ba

Fix for TestMaxSizeWorkersQueue

2c67d0e

dlg99 reviewed Mar 30, 2018

View reviewed changes

eolivelli approved these changes Apr 1, 2018

View reviewed changes

sijie approved these changes Apr 2, 2018

View reviewed changes

dlg99 approved these changes Apr 2, 2018

View reviewed changes

Added microbenchmark

22eba18

Use ImmutableMap

c85ad21

Fixed findbugs configuration to ignore genereted JMH code

9af6ead

merlimat added area/bookie area/client release/4.7.0 labels Apr 3, 2018

merlimat closed this in 46171e6 Apr 3, 2018

merlimat mentioned this pull request Apr 3, 2018

Refactored to reflect changes in BK for OrderedExecutor apache/pulsar#1489

Merged

merlimat deleted the refactor-ordered-executor branch April 3, 2018 05:13

dlg99 mentioned this pull request Jun 21, 2022

Fix the queue size in writeThreadPool exceeds the configured size #3348

Closed

Refactored OrderedSafeExecutor and OrderedScheduler #1309

Refactored OrderedSafeExecutor and OrderedScheduler #1309

Uh oh!

Conversation

merlimat commented Mar 29, 2018

Modifications

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvrao commented Mar 30, 2018

Uh oh!

jvrao commented Mar 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sijie commented Apr 1, 2018

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

merlimat commented Apr 2, 2018

Uh oh!

dlg99 commented Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dlg99 commented Apr 2, 2018 •

edited

Loading