WIP: JDK-8221691: CSS font caching could be improved #424
Conversation
|
CSS is a very complex area, and any performance optimization runs the very real risk of introducing a regression (we have several examples where this has happened). This is especially true when making changes that avoid redundant work. The challenge is to prove that the skipped computation is actually unneeded in all cases. I presume you have run all of the existing test cases? A full, automated test run, including robot tests, plus a manual run of Ensemble8 testing various apps would be a minimum. Before we review the code, please provide a self-contained (in this PR) evaluation of the performance problem and your solution. Also, I would like to see a discussion of the trade-offs and risks associated with doing this. In particular:
|
|
@dgrieve your input would be welcome if you have a few minutes to take a look at this and comment on it. |
|
Did you consider changing the lookupFont method such that it never returns SKIP? lookupFont is an expensive little bugger since it considers all font, font-weight, font-size, and font-style styles up to the root (if necessary). If it returns SKIP, then (worse case) the code goes about calling getCachedFont on parents. If lookupFont returns SKIP and the parent's font isn't cached, then won't parent.lookupFont also return SKIP? So perhaps lookupFont should always cache a font for the given pseudoclass state and never return SKIP. |
MotivationI first became aware of the performance problem when the application thread hung for minutes at a time. After profiling, I found it was spending most of its time evaluating fonts. I couldn't replicate the problem exactly outside our application. It seemed to be that we were doing something in response to a scene property changing (we now execute a run later there) which I do not recommend. In investigating the problem I discovered that many fonts were being recalculated despite an existing font caching mechanism. I designed an application to highlight the problem: import java.util.ArrayList;
import java.util.concurrent.ExecutionException;
import javafx.application.Application;
import javafx.application.Platform;
import javafx.beans.property.SimpleStringProperty;
import javafx.beans.property.StringProperty;
import javafx.beans.value.ObservableValue;
import javafx.concurrent.Task;
import javafx.scene.Scene;
import javafx.scene.control.TableColumn;
import javafx.scene.control.TableView;
import javafx.scene.layout.StackPane;
import javafx.stage.Stage;
import javafx.util.Callback;
public class FontSizeCssTest extends Application {
int columns = 5;
int numRows = 15;
StackPane root = new StackPane();
StackPane tableParent;
@Override
public void start(Stage primaryStage) {
Scene scene = new Scene(root, 800, 600);
primaryStage.setTitle("Hello World!");
primaryStage.setScene(scene);
primaryStage.show();
root.setStyle("-fx-font-size:8pt;");
StackPane curr = new StackPane();
curr.setStyle("-fx-font-size:1em;"); //removing improves performance
root.getChildren().add(curr);
//performance is related to the depth of the scene graph
for (int i = 0; i < 50; i++) {
StackPane child = new StackPane();
curr.getChildren().add(child);
child.setStyle("-fx-font-size:1em;"); //this makes the results significantly worse
curr = child;
}
tableParent = curr;
Thread testThread = new Thread(() -> {
//warmup
for (int i = 0; i < 2; i++) {
withFontSize();
//noFontSize();
}
//actual test
long start = System.currentTimeMillis();
withFontSize();
System.out.println("With setting font size: " + (System.currentTimeMillis() - start));
start = System.currentTimeMillis();
noFontSize();
System.out.println("Without setting font size: " + (System.currentTimeMillis() - start));
});
testThread.start();
}
public void noFontSize() {
for (int i = 0; i < 500; i++) {
TableView table = getNewTestTableview();
Platform.runLater(() -> {
tableParent.getChildren().add(table);
});
waitForUIRender();
Platform.runLater(() -> {
tableParent.getChildren().clear();
});
}
}
public void withFontSize() {
for (int i = 0; i < 500; i++) {
TableView table = getNewTestTableview();
table.setStyle("-fx-font-size:8pt;");
Platform.runLater(() -> {
tableParent.getChildren().add(table);
});
waitForUIRender();
Platform.runLater(() -> {
tableParent.getChildren().clear();
});
}
}
public TableView<StringProperty[]> getNewTestTableview() {
TableView<StringProperty[]> tableView = new TableView<>();
for (int i = 0; i < columns; i++) {
final int colIndex = i;
TableColumn<StringProperty[], String> col = new TableColumn<>();
col.setCellValueFactory(new Callback<>() {
@Override
public ObservableValue<String> call(TableColumn.CellDataFeatures<StringProperty[], String> param) {
return param.getValue()[colIndex];
}
});
tableView.getColumns().add(col);
}
ArrayList<StringProperty[]> rows = new ArrayList<>();
for (int i = 0; i < numRows; i++) {
StringProperty[] row = new StringProperty[columns];
for (int j = 0; j < columns; j++) {
row[j] = new SimpleStringProperty("Row " + i + " Col " + j);
}
rows.add(row);
}
tableView.getItems().addAll(rows);
return tableView;
}
/**
* @param args the command line arguments
*/
public static void main(String[] args) {
launch(args);
}
public void waitForUIRender() {
waitForAppThread();
try {
Thread.sleep((long) (1000.0 / 60));
} catch (InterruptedException ex) {
}
waitForAppThread();
}
private void waitForAppThread() {
Task t = new Task() {
@Override
protected Object call() throws Exception {
return null;
}
};
Platform.runLater(t);
try {
t.get();
} catch (InterruptedException | ExecutionException ex) {
}
}
}This test app adds a table to the bottom of a stack of panes, waits for the app thread, removes the table and repeats the process as fast as possible. There are 2 variations. The first variation is setting the font size at the root node only, meaning that the fonts of all the texts in the table have to be evaluated by traversing all the way up to the root at least once (but without the fix, they do it more than once). The second variation sets the font size on the table to an absolute font size so that the texts dont have to traverse all the way to get to resolve the font. This represents approximately the best performance we could get. Running this app without the fix: With setting font size: 10675ms After the changes to CssStyleHelper: With setting font size: 10852ms SolutionWith the current implementation, the font is cached only when transitionToState() is called on a node. transitionToState() needs to get the font in the current pulse, and to do this it often needs to evaluate the parent node's font recursively. If the parent node's transitionToState gets called first, then its font will be cached and used when evaluating the child(ren) font(s). I don't know if this ever happens, but I know that often the children get evaluated first. If many children get evaluated first, each requiring a full traversal up the scene graph, then there can be a performance impact, depending on the depth of the scene graph. The options are either to change the order which transitionToState is called, which will naturally lead to fonts being cached, or to cache the fonts as we go in the recursive call up the scene graph. I have 2 implementations of caching as fonts are being resolved. The simplest can be found here: DeanWookey@e12f00f. It simply caches the result in the same way it's cached in transitionToState(). The problem however is that now we're caching in 2 places, and it also highlights the problem that getCachedFont was either returning a non-null cached value, or null, which complicates reasoning and means that all code calling getCachedFont has to deal with 2 different cases. The second implementation (the one here) moves all the font resolution and caching into one place. Now we always return the cached version of the font. The idea being that all the other code was already dealing with both cases, so if we eliminate one it shouldn't cause a problem, and it'll help with future work in the area as reasoning about how things work will be easier. Regression TestingI can't think of a way to test that this additional caching, or the fact that the return values are slightly different will cause problems. I will think about it some more. BenefitsAny application adds nodes often would probably benefit from this, particularly if the scene graph is deep. RisksAs mentioned, any changes to the css are risky. While this might not be the biggest performance improvement, these things add up. Our application for example works as a kind of a browser for a server based application. It's noticably slower than its predecessor written in Delphi when the user interacts with it. Our profiling suggests that most of the delays come from the application thread. CSS in particular. The biggest risk with this change is that the code doesn't behave exactly as it used to in some cases. In other words, where there was a call to getCachedFont, now the result can only be a cached value (but it could always have been a cached value if transitionToState() was called in a certain order - unless it never was called in that order in practice). Test ResultsI've run all the tests as you described. I do get failing tests, but I get the same tests failing whether using the latest from the develop branch or with the changes. The following are failing for me (probably because I'm not doing webkit): NullCCLTest. testHTMLEditor I haven't run the ensemble app yet. I'm still trying to figure out the easiest way to get it running using the newly built javafx jars. Any pointers to documentation or tips would be appreciated. |
I didn't really consider that. I was focused on minimising the impact of the changes and lookup font is quite complex. Lookupfont did make a called to getCachedFont on the parent node before. Now that call is getFont and the result is cached, which improves things. I wouldn't be comfortable changing lookupfont without fully understanding the implications at the moment. |
|
I managed to run ensemble with an without the modifications. I couldn't really find anything wrong, but ensemble doesn't really play around with fonts much as far as I can see. |
|
Some things to test that have caused issues in the past:
|
kevinrushforth
changed the title
|
As announced in this message, the official This sandbox repository is being retired on 1-Oct-2019. You will need to migrate your WIP pull request to the openjdk/jfx repo if you want to continue working on it. Here are instructions for migrating your pull request. The updated CONTRIBUTING.md doc has additional information on how to proceed. The new openjdk/jfx repo will be open for pull requests on Wednesday, 2-Oct-2019. I will send email to the openjfx-dev mailing list announcing this. |
|
As mentioned in the bug report https://bugs.openjdk.java.net/browse/JDK-8221691, the recent change to css resolution when nodes are added to the scene graph makes this problem far less of an issue. This problem only occurs if there isn't already a font cached and nodes have to look up the scene graph to resolve them. Now when nodes are added, css is evaluated top down (usually). I'm no longer sure it's worth moving this pull request to the new repo. I tested by putting an assert false where the font would've been looked up but not cached. Most of the tests that failed had something to do with table cells and a call to applyCss(), so there is still a chance there may be a case where this offers a big improvement. Also, like I mentioned, I originally picked up this problem with pulses that were taking minutes. They were triggered (probably) by listening to the scene property of a node and requesting focus when that property changed. It would then evaluate the fonts repeatedly without this caching improvement. I'll move it if others feel it's worth doing, but otherwise I'll leave it. |
Pointer to JBS issue: https://bugs.openjdk.java.net/browse/JDK-8221691
As discussed on the mailing list thread here: https://mail.openjdk.java.net/pipermail/openjfx-dev/2019-March/023157.html, this is what I believe is a better version of the fix here: DeanWookey@e12f00f
The idea is the same except the caching happens in one place instead of multiple places.
The only functional differences are the calls to getFont at the new line indexes: 1392 and 1688. getCachedFont used to return either a cached value which could never be null, or null. Now getFont never returns null so there is a difference. I believe this is fine because some amount of the time the value would be cached and return a non-null value of new CalculatedValue(Font.getDefault(), null, false);
What I'm saying is that the calling code was already dealing with both outcomes just fine, so nailing it down to just one outcome should be fine too. It's also desirable for reasoning about the code in the future.