Optimise FutureConverters.toScala #89
Conversation
Optimise `FutureConverters.toScala` for case when `CompletionStage` backed by `CompletableFuture`
|
@t3hnar I'm sorry, but this seems to have fallen through the cracks. now it needs a rebase, because of merge conflicts in the |
|
@SethTisue done |
|
the compilation failures are 2.13-only and unrelated to this PR, see #97 |
|
now we need a reviewer, maybe @retronym? |
|
review by @viktorklang? |
| @@ -78,6 +79,8 @@ object FutureConverters { | |||
| def toScala[T](cs: CompletionStage[T]): Future[T] = { | |||
| cs match { | |||
| case cf: CF[T] => cf.wrapped | |||
| case cf: CompletableFuture[T] if cf.isDone => | |||
| Future fromTry Try(cf.get()).recoverWith { case e: ExecutionException => Failure(e.getCause) } | |||
There was a problem hiding this comment.
Given this code it is not clear to me that this is in fact an optimization. Is there any benchmark performed that shows what improvement this change leads to?
There was a problem hiding this comment.
@viktorklang Might be I'm missing something, but I believe that optimised version works faster because of removal of possible context switch
There was a problem hiding this comment.
@t3hnar I suspect this to be slower: there will be one allocation for the thunk to Try.apply, then another thunk for the recoverWith function. For the other code it will be a single allocation of the P.
|
*@t3hnar* which benchmark shows what improvement?
--
Cheers,
√
|
|
@viktorklang there is no benchmark, just a blind believe, so feel free to reject this pr. |
Optimise
FutureConverters.toScalafor case whenCompletionStagebacked byCompletableFuture