Performance considerations of syncing FX Nodes with NG Nodes

[Kevin Rushforth]

A longer answer will take more time than I have at the moment, but here 
is a quick reply.

1. We don't have a lot of scene graph performance tests, so before 
making any changes here we would need to do some targeted testing.

2. FX scene graph node --> NGNode peer is intended to be be one-way

3. There are certainly use cases for reusing nodes (attach / detach). 
VirtualFlow will do this for ListView, TableView, etc., with many rows. 
I would expect some apps to make use of it as well, since it's cheaper 
than recreating the FX nodes. There are also cases where nodes are moved 
from one parent to another and might transiently be "removed" from the 
graph.

4. Setting the node's peer to null when detached will cause it to be 
recreated when / if the node is added back into the scene graph, which 
you touch on later in your email. We wouldn't want the case of a 
scrolling ListView / TableVIew to have to continually recreate peers as 
the cells are cycled through.

Anything done here would need to be done very carefully to ensure both 
correctness and performance. Your idea of splitting the peer data from 
the graph connectivity is interesting, but it also sounds pretty intrusive.

Before going down that route I'd want to understand the problem better 
and see if there was a simpler solution to clear the parent / child 
relationship in the render graph.

-- Kevin


On 2/2/2023 9:17 AM, John Hendrikx wrote:
> I have a few questions that maybe some Prism/SceneGraph experts could 
> help me with.
>
> 1) Are there any performance tests related to syncing the NGNode 
> peers? Specifically, I'm interested if there are tests that compare 
> the performance of "fresh" FX graphs that have never been displayed 
> before, versus ones that have their peers already created.
>
> 2) Does prism code ever refer back to FX Nodes?  I've noticed that 
> NGGroup imports javafx.scene.Node for one of its method signatures, 
> but that seems to be a mistake; it can easily be changed to not 
> require javafx.scene.Node.  Aside from several enums, constants and 
> data classes (like Color) being re-used from the javafx side, it seems 
> the NG Prism nodes are well encapsulated and that references are one 
> way only (FX Nodes refer to NG Nodes via `peer`, but never the other 
> way around).
>
> 3) How common is it to re-use FX Nodes that are no longer part of an 
> active scene?  I've found myself that it is unwise to detach/recreate 
> children in high performance controls that reuse cells -- it's often 
> better to just hide cells that are not currently needed instead of 
> removing them from the children list.
>
> 4) Are there any (major) performance implications to setting the NG 
> peer to `null` when FX nodes are not part of an active sync cycle (ie, 
> they have no Scene, or the Scene is not currently visible)?
>
> My observations on the sync cycle (syncPeer/doUpdatePeer) is that it 
> is highly optimized, and tries to avoid new allocations as much as 
> possible.  However, this seems to come at a price: it leaks memory 
> when not part of a sync cycle. Given a FX Node graph, that has been 
> displayed at least once, a mirrored graph is created consisting of 
> NGNodes. When such a FX Node graph is no longer displayed, any changes 
> to it are no longer synced to the mirror.
>
> This means that when I detach a small portion of the FX Node graph 
> (let's say a single Node), and keep a reference to only that Node, 
> that the FX graph can be GC'd. The corresponding NG Node graph however 
> (which still hasn't been updated) can't be GC'd. The single detached 
> FX Node has a peer, and this peer has a parent (and children), keeping 
> not only the detached Node's peer in memory, but also all the other 
> peers in the mirrored graph.
>
> Notifying the NG mirrored graph of changes is not easy, as it must be 
> done as part of the sync (locking is involved). Even a detached FX 
> Node can't assume its peer can be modified without locking as it may 
> still be used in an active rendering pass.
>
> This leads me to believe that to move to a situation where peers are 
> not being leaked would involve clearing peers as soon as FX nodes 
> becomes detached from the sync cycle.
>
> This has no doubt has performance implications, as peers would need to 
> be recreated if nodes are re-used (see Q3).  However, not all data 
> that is part of the peers is a problem, and code that simply clears 
> its peer when detached could be optimized again to be more peformant 
> (optimizing from a correctly working situation, instead of fixing 
> problems working from an optimized situation that has memory leaks).
>
> One way to optimize this may be to split the data that is tracked by 
> peers in parts that are just direct copies of FX Node data, and parts 
> that refer to parent/children.  The data that is just copies could be 
> kept ("peerData") while the peer itself is nulled.  When the peer 
> requires recreation, it is constructed as "new NGNode(peerData)".  
> Because the parent/child linkages are not part of `peerData`, it is no 
> longer possible for large NG node trees to be kept in memory.  
> Recreation of NGNode would still require work, but these NGNodes are 
> much smaller.
>
> --John
>