Performance considerations of syncing FX Nodes with NG Nodes

[John Hendrikx]

On 03/02/2023 01:27, Kevin Rushforth wrote:
> 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.
I completely agree here, if there are no tests that can be re-used then 
some will need to be created, that's okay.
>
> 2. FX scene graph node --> NGNode peer is intended to be be one-way
Makes sense.
>
> 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.
Moving nodes is one of the cases that may degrade in performance. 
Keeping (unused) cell nodes as part of the children list but 
invisible/unmanaged should not be affected (I'm not 100% sure what 
VirtualFlow is doing at the moment, I only know that's how I do it in my 
own ListViewSkin -- I don't think it had a performance reason, more that 
it could result in visual artifacts in 1 rendered frame before correct 
colors were applied -- perhaps a bug in itself).
>
> 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.
Yeah, not sure if that is what would happen, it depends on how 
VirtualFlow is handling its cells.
>
> 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.

There are multiple problems it seems in this area. At first I thought it 
was only the `removed` list in Parent that is a problem, as it can 
contain old Nodes that don't get cleaned up when not part of a sync 
cycle.  But any node that is not part of a sync cycle may refer to a 
peer, and that peer can refer to parent/children in turn, keeping an 
entire NGNode graph in memory.

For the `removed` list in Parent, there is a simple solution I think.  
It already has a mechanism to mark itself as "fully" dirty when more 
than 20 children are removed. This mechanism could be re-used to also 
mark it as fully dirty when the Parent is no longer part of an active 
sync cycle. If ever re-attached, it would just fully render instead of 
trying to calculate a smaller dirty region based on removed children.

There is also an odd piece of code in Node, in invalidateScenes. In this 
code it will call `peer.release()` which does nothing for any of the 
current NG implementations (even though there are comments that they 
should do "something").  But because it doesn't set `peer` to `null` it 
will later use the "released" peer again if re-attached to a scene.  If 
this is not a mistake, it deserves an explanatory comment I think.

--John


>
> -- 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
>>
>