JDK-8164933 : Charts are layed out on each pulse
  • Type: Bug
  • Component: javafx
  • Sub-Component: scenegraph
  • Affected Version: 9
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2016-08-29
  • Updated: 2016-11-24
  • Resolved: 2016-11-24
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 9
9Fixed
Related Reports
Relates :  
JDK-8137252 - JavaFX StackPane bounds not updated
Relates :  
JDK-8170317 - TextFlow is layed out on each pulse
Description
This is a performance regression from JDK-8137252.
Run hello.HelloBarChart sample with -Djavafx.pulseLogger=true
After that fix there is a constant stream of pulses.
If there are several charts in an app, it consumes a single core on my i5.
Comments
Changeset: d06065b38e3a Author: vadim Date: 2016-11-24 08:33 +0300 URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/d06065b38e3a
24-11-2016
+1. I agreed with you evaluation and your proposed fix looks good. Can you please file a JIRA for TextFlow too? Thanks!
22-11-2016
Here's the updated fix with the PieChart fixed: http://cr.openjdk.java.net/~vadim/8164933/webrev.01/ Unfortunately, for the reason stated in the above comment TextFlow will suffer from this and it seems impossible to fix without making the new requestLayout(boolean) method public. BTW, I tried it and it basically fixes all these issues. I don't think we can add new public methods though...
17-11-2016
Actually the root cause of this behaviour is this: In the fix for JDK-8137252 new method Parent.requestLayout(boolean) was introduced and it is called in some cases instead of old Parent.requestLayout() But some classes (Axis and TextFlow are the examples) override requestLayout and do some tricks to modify layout behaviour. So before the fix for example Node.doNotifyLayoutBoundsChanged was calling requestLayout and if parent has it overridden, it modified the behaviour. But now the requestLayout(boolean) is called so overridden method is not called hence the bug.
16-11-2016
This fix looks good but it didn't completely resolve the issue in all cases of Charts. I still see continuous pulse when I select "Pie Chart" or "Drilldown Pie Chart" running Ensemble8. It is likely we need similar fix in PieChart class.
25-10-2016
Here's the fix for the issue: http://cr.openjdk.java.net/~vadim/8164933/webrev.00/ I've extracted relevant portions of the code to new methods so as not to indent the code even further. The new updateTickMarks method in the Axis is moved under the existing dirty condition, new updateMinorTickPath in the ValueAxis is guarded by the minorTickMarksDirty field.
25-10-2016
Based on my discussion with Vadim he has an upcoming fix that might partially address this update issue and he is willing to look into fixing the rest as well.
17-10-2016
I have done more investigative work into Chart and found that there are 2 areas that lead to Chart triggers layout on each pulse: 1) Axis.positionTextNode() always set and reset Text node's layoutX/Y with new computed values. 2) tickMarkPath (in Axis.updateTickMark()) and minorTickPath (in ValueAxis.layoutChildren()) is always cleared and recomputed at each layout. Both type of operations will generate a pulse request for next frame. Looks like Chart needs a better dirty bit scheme that can short-circuit layout computation if Chart is not dirty.
17-10-2016
I have narrowed down the portion of the fix that introduces this regression. However this portion of the fix is essential to the overall fix to JDK-813752. Hence we may have uncovered another bug with the fix to JDK-813752. We need to do some benchmarking to determine the severity of this bug. Here is the diff that revert the portion of the fix: diff --git a/modules/javafx.graphics/src/main/java/javafx/scene/Parent.java b/modules/javafx.graphics/src/main/java/javafx/scene/Parent.java --- a/modules/javafx.graphics/src/main/java/javafx/scene/Parent.java +++ b/modules/javafx.graphics/src/main/java/javafx/scene/Parent.java @@ -1199,12 +1199,8 @@ * Calling this method while the Parent is doing layout is a no-op. */ public final void layout() { - // layoutFlag can be accessed or changed during layout processing. - // Hence we need to cache and reset it before performing layout. - LayoutFlags flag = layoutFlag; - setLayoutFlag(LayoutFlags.CLEAN); - switch(flag) { - case CLEAN: + switch(layoutFlag) { + case CLEAN: break; case NEEDS_LAYOUT: if (performingLayout) { @@ -1232,6 +1228,7 @@ } } currentLayoutChild = null; + setLayoutFlag(LayoutFlags.CLEAN); performingLayout = false; break; }
12-10-2016