[jira] [Comment Edited] (TINKERPOP-2393) BranchStep should respect the barrier size of BarrierStep when calling applyCurrentTraverser

["redtree (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/TINKERPOP-2393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17178364#comment-17178364 ] 

redtree edited comment on TINKERPOP-2393 at 8/15/20, 6:43 PM:
--------------------------------------------------------------

[~spmallette] Thank you for the reply. 

Yeah in another thread I noticed that having more barrier steps will break Aggregate SideEffect's locality more, so it's not so simple to have the batch call functionality in more steps natively...

I think for now we should aim for the 1st approach. But now I noticed that currently BranchStep determines if it has a barrier during construction

[https://github.com/apache/tinkerpop/blob/534746b6019d0775e628375a3dfeba8a18df90fe/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/BranchStep.java#L92]

This doesn't work for our case as we swap PropertiesStep with our own BulkPropertiesStep which would implement Barrier when TraversalStrategies are applied, so it is too late.
 I think BranchStep should defer this check during query run-time, luckily it already has the flag "first" , so only for the first time we can call this

{{if (!this.hasBarrier && !TraversalHelper.getStepsOfAssignableClassRecursively(Barrier.class, traversalOption).isEmpty())}}{\{  }}

{{  this.hasBarrier = true;}}

in StandardAlgorithm. By this change BranchStep also detects any NoOpBarrierStep added for optimization but it should be ok.
 Does it make sense to you ? I first want to make this change then. 


was (Author: redtree1112):
[~spmallette] Thank you for the reply. 

Yeah in another thread I noticed that having more barrier steps will break Aggregate SideEffect's locality more, so it's not so simple to have the batch call functionality in more steps...

I think for now we should aim for the 1st approach. But now I noticed that currently BranchStep determines if it has a barrier during construction

[https://github.com/apache/tinkerpop/blob/534746b6019d0775e628375a3dfeba8a18df90fe/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/BranchStep.java#L92]

This doesn't work for our case as we swap PropertiesStep with our own BulkPropertiesStep which would implement Barrier when TraversalStrategies are applied, so it is too late.
I think BranchStep should defer this check during query run-time, luckily it already has the flag "first" , so only for the first time we can call this

{{if (!this.hasBarrier && !TraversalHelper.getStepsOfAssignableClassRecursively(Barrier.class, traversalOption).isEmpty())}}{{  }}

{{  this.hasBarrier = true;}}

in StandardAlgorithm. By this change BranchStep also detects any NoOpBarrierStep added for optimization but it should be ok.
Does it make sense to you ? I first want to make this change then. 

> BranchStep should respect the barrier size of BarrierStep when calling applyCurrentTraverser
> --------------------------------------------------------------------------------------------
>
>                 Key: TINKERPOP-2393
>                 URL: https://issues.apache.org/jira/browse/TINKERPOP-2393
>             Project: TinkerPop
>          Issue Type: Improvement
>          Components: process
>    Affects Versions: 3.4.7
>            Reporter: redtree
>            Priority: Minor
>
> Right now in BranchStep, if children contain a BarrierStep it keeps calling applyCurrentTraverser until this.starts is exhausted.
> [https://github.com/apache/tinkerpop/blob/534746b6019d0775e628375a3dfeba8a18df90fe/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/BranchStep.java#L139-L140]
> This means the child step may hold all upstream solutions in memory at once, and potentially memory usage is bloated.
> I think here we can change so that BarrierStep can have a method like getMaxBarrierSize, and BranchStep examines the size of barrier that child steps can handle at once then break the while loop either when `this.starts.hasNext` becomes false or we exceed the barrier size to save the potential memory consumption.
>  
> When there are multiple barrier steps as children, we may pick either max or min among them.
> I am happy to implement this, but please let me know if you have any comments or concerns on this change. 
> Thanks



--
This message was sent by Atlassian Jira
(v8.3.4#803005)