visgl / deck.gl

WebGL2 powered visualization framework
https://deck.gl
MIT License
12.3k stars 2.09k forks source link

Data accessor methods shouldn't be invoked for hidden layers #6056

Open Anand2489 opened 3 years ago

Anand2489 commented 3 years ago

Target Use case

It would be a big performance improvement if DeckGL doesn't invoke data accessors for hidden layers as they run on the CPU (Official docs)

Any specific reason for not doing so?

Proposed feature

If a layer isn't visible, it's data accessor methods shouldn't be invoked

To Do List

Pessimistress commented 3 years ago

I agree this is a reasonable expectation, though there are some complications if we decide to pursue this.

A naïve implementation would be to block attributeManager from updating if props.visible is false. That would prevent most accessor calls by putting off attribute updates.

However, a layer could also call accessors in updateState outside of the attributeManager. Examples of this include the IconLayer and TextLayer auto-packing atlas, TileLayer fetching new tiles, aggregation layers updating aggregations, etc.

If we skip updateState all together when a layer is not visible, then we need to make sure that all the skipped operations are performed once it becomes visible again. Most layers' updateState rely on comparing oldProps and props to determine what needs update. The assumption is that the most recent version of the layer (oldProps) already made all the updates required by its props, which would be broken by the proposed change.

Another issue is that there are multiple ways to control the visibility of a layer, including Layer.props.visible, CompositeLayer.filterSubLayer and Deck.props.layerFilter. The latter two are called before redraw and are dependent on the current viewport/pass. They cannot be evaluated accurately during layer updates. For example, if a layer is only rendered in a minimap and the minimap is turned off, there is no way for the layer to know that it is no longer visible.

I've also seen use cases where an app hides a layer until it's fully loaded. This can be worked around by switching to layerFilter, though it would be a breaking change.

ibgreen commented 3 years ago

If we skip updateState all together when a layer is not visible, then we need to make sure that all the skipped operations are performed once it becomes visible again.

To tackle this in a general way, at one point I started adding a "change flags" system to the layers. The idea was that that various operations would set these flags to mark various aspects of the layer as "dirty", but actual updates to the layers could be delayed until needed.

Unfortunately, there was a lot of churn in the libraries at the time, so in the interest of stability I paused the refactor, and never got back to it.

I believe the flags are still in the code. However the approach was likely not fully developed - i.e. there are still some design problems to be worked through (and perhaps the deck.gl of today would need an excessive set of flags, or a fixed set of flags can no longer cover our needs, ...).

Pessimistress commented 3 years ago

@ibgreen The props diffing function aborts when the first change is detected and set the propsChanged flag, therefore we cannot rely on the flags to tell which props have changed. In updateStates layers still have to use oldProps.someProp !== props.someProp to check if a specific prop has changed.

ibgreen commented 3 years ago

True. There is more thinking to be done. FWIW, I never expected this change to be done without modifying layer updateState functions (in a backwards compatible way, of course).

Anand2489 commented 3 years ago

@Pessimistress How about this check

shouldUpdateState ({props, oldProps}) {
    const shouldUpdate = props.visible || oldProps.visible
    return shouldUpdate
  }

In case layer is gonna be invisible in next update cycle, oldProps.visible is gonna take care for accessor clean-up etc.

I tried this with TextLayer & it worked for me

Anand2489 commented 3 years ago

@ibgreen @Pessimistress Any update on above pointer? What do you guys think about the above check in shouldUpdateState lifecycle hook

Pessimistress commented 3 years ago

A change like this will have a cascading effect on use cases such as tiling, multi-view, and transition. There is also the concern of holding on to outdated resources in memory if we don't immediately flush the change flags.

I am not sure what scenario you are trying to address here - attribute updates are only triggered by 1) data change 2) update triggers. If your layer is invisible, why update its props in the first place?

You can always extend the build-in layers with a custom shouldUpdateState method if your application does not touch the above mentioned edge cases.

juneidy commented 2 years ago

I am seeing the behaviour where my tile layer is downloading tiles even when it's not visible. Is it related to this issue?