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

feat: extend VertxImpl to allow for Context scheduling interception #1787

Closed
wants to merge 1 commit into from

Conversation

@emmanuelidi
Copy link
Contributor

emmanuelidi commented Jan 30, 2017

resolves: issue #1785
Signed-off-by: Emmanuel Idi emmanuel.idi@gmail.com

resolves: issue #1785
Signed-off-by: Emmanuel Idi <emmanuel.idi@gmail.com>
@vietj vietj added the to review label Jan 30, 2017
* @return a reference to this, so the API can be used fluently
*/
@GenIgnore
Vertx addSchedulerInterceptor(BiFunction<Context, Runnable, Runnable> interceptor);

This comment has been minimized.

Copy link
@cescoffier

cescoffier Jan 31, 2017

Contributor

At some point we should discuss supporting this in a polyglot way (meaning supporting BiFunction in codegen).

ctx.executeBlocking(f -> {
assertEquals(2, cnt.get());
testComplete();
}, true, null);

This comment has been minimized.

Copy link
@cescoffier

cescoffier Jan 31, 2017

Contributor

You should also check when the callback go back to the event loop context.

});
vertx.runOnContext(v -> {
assertEquals(2, cnt.get());
testComplete();

This comment has been minimized.

Copy link
@cescoffier

cescoffier Jan 31, 2017

Contributor

A very similar test with vertx.setTimer and vertx.setPeriodic and vertx.setPeriodicStream would be beneficial.

This comment has been minimized.

Copy link
@cescoffier

cescoffier Jan 31, 2017

Contributor

Sorry they are below, only the stream version is missing.

return r;
});

ctx.runOnContext(v -> {

This comment has been minimized.

Copy link
@cescoffier

cescoffier Jan 31, 2017

Contributor

We should try entangled contexts (2 differents contexts)

});

await();
}

This comment has been minimized.

Copy link
@cescoffier

cescoffier Jan 31, 2017

Contributor

I would also add a test when reading read stream with back pressure (pause /resume).

});

await();
}

This comment has been minimized.

Copy link
@cescoffier

cescoffier Jan 31, 2017

Contributor

Once the event bus interceptors are merged, we should also have a test between 2 verticles, and 1 regular verticle and 1 worker verticle.

@Override
public Runnable interceptScheduledWork(final ContextImpl context, final Runnable task) {
return schedulerInteceptors.stream()
.reduce((c, t) -> t, (a, b) -> (c, t) -> b.apply(c, a.apply(c, t)))

This comment has been minimized.

Copy link
@cescoffier

cescoffier Jan 31, 2017

Contributor

I found this line elegant but also confusing to read. Maybe a more imperative version would improve readability (and also improve perf a bit).

This comment has been minimized.

Copy link
@emmanuelidi

emmanuelidi Jan 31, 2017

Author Contributor

Just an FYI. Since the set of interceptors is likely to be fixed after startup, I had wanted to cache the composition of the interceptors as an optimization (since, now, I'm rebuilding on each task schedule which feels pretty stupid). I didn't to avoid the cache maintenance logic and just minimize the number of changes while evaluating the concept.

});

await();
}

This comment has been minimized.

Copy link
@cescoffier

cescoffier Jan 31, 2017

Contributor

I would also add a test providing a kind of "locals" feature (like ThreadLocal but Task Local). It would validate the provided API.

@vietj

This comment has been minimized.

Copy link
Member

vietj commented Jan 31, 2017

hi @emmanuelidi thanks for contrib, I may rework some parts to make the interception a bit differently but provide some equivalent.

@emmanuelidi

This comment has been minimized.

Copy link
Contributor Author

emmanuelidi commented Jan 31, 2017

@vietj no problem. Do you want me to work on any of the above comments or are you planning to take it from here? If you need my help, I should be able to make some updates later today

@vietj

This comment has been minimized.

Copy link
Member

vietj commented Jan 31, 2017

@emmanuelidi I don't know yet, I need to look at your modification and hopefull make some benchmarks to see how it goes to see if there are regressions

@vietj

This comment has been minimized.

Copy link
Member

vietj commented Feb 1, 2017

I've pushed in my branch a JMH benchmark (that you can review and criticize) : https://github.com/vietj/vert.x/tree/emmanuelidi-issue-1785

the current overhead is quite high for the default case:

Benchmark                   Mode  Cnt       Score      Error   Units
Benchmark1.baseline        thrpt   15  228082.881 ± 5827.380  ops/ms
Benchmark1.noInterceptor   thrpt   15   37038.989 ± 1078.036  ops/ms
Benchmark1.oneInterceptor  thrpt   15   26951.174 ± 1054.282  ops/ms

I've modified the Vert.x Context to run a tasks directly and not on the event loop (as it does not make sense in this case).

In the noInterceptor case we should have the same score than in the baseline case and in the oneInterceptor case we should be as high as possible.

I've run it with:

> java -Dvertx.disableTCCL=true -Dvertx.disableContextTimings=true -Dvertx.threadChecks=false -jar target/benchmarks.jar

I'll do some updates in the branch to see how it can be improved, that being said anyone can also do that on their side and most importantly criticize the JMH benchmark I wrote because I may have done wrong things (and it is very likely actually).

@vietj

This comment has been minimized.

Copy link
Member

vietj commented Feb 1, 2017

note we should add new methods on the Context like I've done, so we can compare all implementations.

@vietj

This comment has been minimized.

Copy link
Member

vietj commented Feb 1, 2017

I pushed extra implementations aiming to improve the default case:

  1. use interceptScheduledWork method
  2. use a BiFunction<Context, Runnable, Runnable> that returns the runnable when empty otherwise interceptScheduledWork
  3. same as 2. but cache the BiFunction<Context, Runnable, Runnable> in ContextImpl : avoids a volatile read on the BiFunction
Benchmark                    Mode  Cnt       Score       Error   Units
Benchmark1.baseline         thrpt    5  235832.507 ± 13439.406  ops/ms
Benchmark1.noInterceptor1   thrpt    5   38116.302 ±  1286.238  ops/ms
Benchmark1.noInterceptor2   thrpt    5  202753.564 ±  5281.661  ops/ms
Benchmark1.noInterceptor3   thrpt    5  225995.015 ±  4744.601  ops/ms
Benchmark1.oneInterceptor1  thrpt    5   27782.837 ±   702.241  ops/ms
Benchmark1.oneInterceptor2  thrpt    5   29447.137 ±  3068.974  ops/ms
Benchmark1.oneInterceptor3  thrpt    5   29798.258 ±  1214.843  ops/ms
@vietj

This comment has been minimized.

Copy link
Member

vietj commented Feb 1, 2017

I've added a fourth case

  1. use interceptScheduledWork method
  2. use a BiFunction<Context, Runnable, Runnable> that returns the runnable when empty otherwise interceptScheduledWork
  3. same as 2. but cache the BiFunction<Context, Runnable, Runnable> in ContextImpl : avoids a volatile read on the BiFunction
  4. like 3. but instead the default value is null
Benchmark                    Mode  Cnt       Score       Error   Units
Benchmark1.baseline         thrpt    5  235209.431 ± 20526.807  ops/ms
Benchmark1.noInterceptor1   thrpt    5   38341.061 ±  3410.366  ops/ms
Benchmark1.noInterceptor2   thrpt    5  203518.193 ±  7209.513  ops/ms
Benchmark1.noInterceptor3   thrpt    5  225073.375 ±  5957.404  ops/ms
Benchmark1.noInterceptor4   thrpt    5  236281.564 ±  1783.494  ops/ms
Benchmark1.oneInterceptor1  thrpt    5   27840.205 ±   711.348  ops/ms
Benchmark1.oneInterceptor2  thrpt    5   28662.689 ±  1101.673  ops/ms
Benchmark1.oneInterceptor3  thrpt    5   29550.535 ±   270.043  ops/ms
Benchmark1.oneInterceptor4  thrpt    5   29787.272 ±   311.723  ops/ms
@vietj

This comment has been minimized.

Copy link
Member

vietj commented Feb 1, 2017

here is an update for the case with interceptors:

  • 1/ use interceptScheduledWork
  • 2/ , 3/ , 4/ use directly the BiFunction when there is one interceptor otherwise it copies the array and iterate it with a loop (hence now there is oneInterceptor and twoInterceptors cases)
Benchmark                     Mode  Cnt       Score       Error   Units
Benchmark1.baseline          thrpt    5  229550.793 ± 14749.522  ops/ms
Benchmark1.noInterceptor1    thrpt    5   37270.542 ±  2365.457  ops/ms
Benchmark1.noInterceptor2    thrpt    5  199651.119 ±  7543.540  ops/ms
Benchmark1.noInterceptor3    thrpt    5  219081.480 ± 11843.743  ops/ms
Benchmark1.noInterceptor4    thrpt    5  228470.940 ±  9684.191  ops/ms
Benchmark1.oneInterceptor1   thrpt    5   26994.483 ±  2025.527  ops/ms
Benchmark1.oneInterceptor2   thrpt    5   77672.996 ±  2555.805  ops/ms
Benchmark1.oneInterceptor3   thrpt    5   81922.068 ±  9602.190  ops/ms
Benchmark1.oneInterceptor4   thrpt    5   81208.319 ± 11265.301  ops/ms
Benchmark1.twoInterceptors1  thrpt    5   19905.757 ±   753.210  ops/ms
Benchmark1.twoInterceptors2  thrpt    5   43781.834 ±  4668.595  ops/ms
Benchmark1.twoInterceptors3  thrpt    5   50455.022 ±  1754.322  ops/ms
Benchmark1.twoInterceptors4  thrpt    5   47108.169 ±  2007.464  ops/ms
@vietj

This comment has been minimized.

Copy link
Member

vietj commented Feb 1, 2017

I'm wondering if we need this to be much dynamic (i.e add/remove at runtime) and instead provide an SPI mechanism loaded when Vert.x is created that allows to have an optimal default case with no interceptors are needed.

So just having this jar on the classpath would load the SPI allowing such interception.

@emmanuelidi

This comment has been minimized.

Copy link
Contributor Author

emmanuelidi commented Feb 2, 2017

@vietj That seems reasonable to me. As I was saying above, folding the interceptors and saving the result would be desirable. Using an SPI mechanism would allow for that and avoid the fold-per-schedule which is occurring now (and is wasteful)

@cescoffier

This comment has been minimized.

Copy link
Contributor

cescoffier commented Feb 2, 2017

In the distributed tracing use case, we may decide to trace or not some request (sampling). Despite it could be done in the interceptor itself, to avoid the performance cost it's recommended to register the interceptor only when required. In this case, being able to register interceptors at runtime would be useful.

Another use case, also in the distributed tracing context: being able to trace a specific interaction when something "bizarre" is happening.

So there are use cases where dynamic registration can be something interesting to have.

@vietj

This comment has been minimized.

Copy link
Member

vietj commented Feb 7, 2017

I've created this branch that aims to provide basis for writing task oriented benchmarks that we can use after https://github.com/eclipse/vert.x/tree/task-interception

@vietj

This comment has been minimized.

Copy link
Member

vietj commented Feb 7, 2017

There is something I don't understand in your PR.

I'm supposing such kind of interceptor @emmanuelidi and @cescoffier looks like:

BiFunction<Context, Runnable, Runnable> interceptor = (ctx, task) -> {
   // Capture a contextual object here
   return () -> {
      // Reuse the captured contextal object
      task.run();
   };
};

In this case, I don't see how timers and various channel operations are intended to work.

For instance let's say the timer (although I see a timer test in your code), here is a simplified version of the timer:

void setTimer(long delay, Handler<Void> callback) {
   long timerId = createTimerId();
   Context ctx = vertx.getOrCreateContext();
   ctx.nettyEventLoop().schedule(delay, () -> {
      // When executed the context interceptor will have lost track of the interceptor context
      ctx.runOnContext(v -> {
         callback.handler(timerId);
      });
   });
}

We could fix it for the timer, but the same would happen with any asynchronous Netty operation such as a channel connect, etc...

Does it make sense or is there something I'm missing ?

@adriancole adriancole mentioned this pull request Jan 27, 2018
@stephenh

This comment has been minimized.

Copy link

stephenh commented Sep 10, 2018

Hey guys; curious if this PR/feature could be revived? I have two use cases that need this functionality:

  1. Distributed tracing (I saw Adrian added tracing of the request, but it only handles extraction of incoming requests and not injection into outgoing requests)
  2. Dataloader micro-/automatic-batching, e.g. (vertx-dataloader)[https://github.com/engagingspaces/vertx-dataloader] requires manually calling dataloader.dispatch() and I'd like to have this called at the end of the event loop for every future/callback executed on the event loop.

It seems like the PR was ~close? Although the perf numbers also don't look very good.

@emmanuelidi

This comment has been minimized.

Copy link
Contributor Author

emmanuelidi commented Sep 11, 2018

I've stopped using Vert.x so, unfortunately, the window of time for me to work on this has come and gone.

@vietj

This comment has been minimized.

Copy link
Member

vietj commented Sep 11, 2018

@stephenh we worked on a branch to extend the prototype, it should make vert.x 4 , see https://github.com/vert-x3/wiki/wiki/RFC:-continuation-instrumentation

@stephenh

This comment has been minimized.

Copy link

stephenh commented Sep 11, 2018

@vietj That's great! I will poke around at that branch.

The Vert.x 4 roadmap looks pretty great, e.g. CompletionStage support.

Apologies if this is an annoying question, but what does ~general timeline for 4.x look like? I didn't see in dates in the roadmap, which is understandable, but I'm new to the vertx community, so just don't know if it's like a "we just started" or "wrapping up soon" etc.

@vietj

This comment has been minimized.

Copy link
Member

vietj commented Sep 11, 2018

@stephenh we target the first semester of 2018, but you can expect milestones of course

@vietj vietj closed this Jul 2, 2019
@vietj

This comment has been minimized.

Copy link
Member

vietj commented Jul 2, 2019

cleanup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.