FlatMapPipelinedCursor starts background calculations of pipelined inner cursors
#3072
Conversation
- this should improve the performance of the pipeline now that it schedule up to N-calculations of the inner's first cursor item, where N is the size of the pipeline.
| @@ -295,20 +296,26 @@ public PipelineQueueEntry(RecordCursor<V> innerCursor, | |||
| this.priorOuterContinuation = priorOuterContinuation; | |||
| this.outerResult = outerResult; | |||
| this.outerCheckValue = outerCheckValue; | |||
| // start calculating the next result in the background. | |||
| setInnerFuture(); | |||
There was a problem hiding this comment.
As a matter of style, I think it would be better not to fire off this future in the constructor. There are some security implications in general if this can throw an error (see: https://www.baeldung.com/java-constructors-exceptions, see "Security concerns"), but also, it's counter-intuitive for users of the API, who don't expect constructing an object to have side-effects like this. If we really want to always fire off this future when we make a new element, I think we should make a static initializer, which removes the security concern and is less likely to surprise API consumers (which, granted, is just us because this is a private class)
| @@ -267,6 +267,7 @@ private synchronized void addEntryToPipeline(PipelineQueueEntry pipelineQueueEnt | |||
| if (closed) { | |||
| pipelineQueueEntry.close(); | |||
| } | |||
| pipelineQueueEntry.getNextInnerPipelineFuture(); | |||
There was a problem hiding this comment.
Do we need both this change and the change in the PipelineQueueEntry constructor? If I'm reasoning about this correctly, it seems like we only need one of the two (that this is extraneous if the next future is fired off during the constructor, and the constructor call is extraneous if the future is created here)
There was a problem hiding this comment.
Correct, this was an oversight, let me remove that.
Result of fdb-record-layer-pr on Linux CentOS 7
|
Result of fdb-record-layer-pr on Linux CentOS 7
|
- scheduling the calculation of inner in the pipeline is triggering extra computation.
Result of fdb-record-layer-pr on Linux CentOS 7
|
hatyo
changed the title
FlatMapPipelinedCursor starts background calculations of pipelined inner cursors
This PR enables scheduling the calculation of the first cursor item of inner when as soon as it is added to the pipeline, this should improve the performance of the flat map pipeline cursor in situations where the calculation of inner is much more CPU intensive compared to outer, it comes however with the cost of potentially over computing.