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
