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

DialogueClients takes advantage of the EndpointChannel bind optimization #818

Merged

Conversation

iamdanfox
Copy link
Contributor

@iamdanfox iamdanfox commented Jun 4, 2020

Before this PR

@bavardage reported that he was seeing a lot of the following stacktrace:

13:36:34.102 [ForkJoinPool-1-worker-15] DEBUG com.palantir.conjure.java.dialogue.serde.DefaultClients - Channel of type LiveReloadingChannel does not implement EndpointChannelFactory, which is recommended for maximum performance. Falling back to lambda impl.
com.palantir.logsafe.exceptions.SafeRuntimeException: Exception for stacktrace
	at com.palantir.conjure.java.dialogue.serde.DefaultClients.bind(DefaultClients.java:81)
	at com.palantir.conjure.java.dialogue.serde.DefaultClients.call(DefaultClients.java:55)

This is pretty sad, because it means I spent all this time building the fancy 'bind' optimization in #764, but because LiveReloadingChannel never implemented the magic interface, nobody was actually getting this optimization!!!

Benchmark                                  Mode  Cnt       Score       Error  Units
EndToEndBenchmark.dialogueBlocking        thrpt   16   21813.837 ±   837.945  ops/s
EndToEndBenchmark.rawApacheBlocking       thrpt   16   20436.592 ±   960.568  ops/s
EndToEndBenchmark.setZeroNetworkDialogue  thrpt   16  371603.457 ± 10369.373  ops/s

After this PR

==COMMIT_MSG==
Clients constructed using DialogueClients and modern conjure-java (5.17.0+) now spend slightly fewer CPU cycles on each RPC call, because they use the bind method to do per-endpoint setup once at construction time instead of before every rpc. Benchmarks show a 21k -> 27k request/sec improvement against a trivial ping endpoint.
==COMMIT_MSG==

Benchmark                                  Mode  Cnt       Score       Error  Units
EndToEndBenchmark.dialogueBlocking        thrpt   16   27345.824 ±  1843.425  ops/s
EndToEndBenchmark.rawApacheBlocking       thrpt   16   25026.853 ±  1958.190  ops/s
EndToEndBenchmark.setZeroNetworkDialogue  thrpt   16  376254.738 ± 23785.571  ops/s

Possible downsides?

  • This mistake happened because we're using an instanceof check to detect when to go on the fast path, so the types weren't clear enough. This is probably gonna hit us again unless we fix this.

@changelog-app
Copy link

changelog-app bot commented Jun 4, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Clients constructed using DialogueClients and modern conjure-java (5.17.0+) now spend slightly fewer CPU cycles on each RPC call, because they use the bind method to do per-endpoint setup once at construction time instead of before every rpc. Benchmarks show a 21k -> 27k request/sec improvement against a trivial ping endpoint.

Check the box to generate changelog(s)

  • Generate changelog entry

@policy-bot policy-bot bot requested a review from CRogers June 4, 2020 21:55
@iamdanfox iamdanfox requested review from ferozco and carterkozak and removed request for CRogers June 4, 2020 22:02
@bulldozer-bot bulldozer-bot bot merged commit 8c63614 into develop Jun 4, 2020
@bulldozer-bot bulldozer-bot bot deleted the dfox/actually-use-endpointchannel-optimization branch June 4, 2020 22:14
@svc-autorelease
Copy link
Collaborator

Released 1.56.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants