Improve entry editor first-open performance - #16467
Conversation
PR Summary by QodoImprove entry editor first-open performance with lazy controls and ControlsFX prewarm
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Siedlerchr
added
the
dev: binaries
subhramit
left a comment
subhramit
left a comment
There was a problem hiding this comment.
Code-wise lgtm. Waiting for user tests.
|
Refs. #15962 |
|
Just wanted to add that it is usually best practice to explicitly wrap a scene content node with a |
…irst-open * upstream/main: Support partial citations for JStyles (#16459) Add BibTeX source highlighting (#16328) Chore(deps): Bump org.xmlunit:xmlunit-matchers in /versions (#16474) Chore(deps): Bump org.apache.httpcomponents.client5:httpclient5 (#16473) Chore(deps): Bump org.xmlunit:xmlunit-core in /versions (#16472) Support track-changes mode for LibreOffice Integration (#16458) New Crowdin updates (#16460) Add SLR documentation (#16441) Fix bst duplicate reject (#16388) Update javafx to 26.0.2 (#16465)
|
@crschnick Thanks for the tip, but not sure if I understand this correctly. In ControlsFX (that we still use) DecorationPane is inaccessible from the outside. So we cannot use that directly. We are also looking for moving away from controlsfx validation in #16174 |
|
@Siedlerchr's: Findings of the performance analysis: Entry editor first-open performanceThis report records the baseline Java Flight Recorder (JFR) evidence for the reported UI freeze when the entry editor is opened for the first time. It accompanies the raw recording so the evidence can be inspected again and compared with later implementations. The investigation is tracked in JabRef pull request #16464. ResultThe recording supports eager field-editor construction followed by JavaFX CSS and layout work as the main source of the first-open stall.
These spans are evidence windows between statistical samples, not exact method durations. The recording did not include a custom event marking the menu action or JavaFX pulse-duration events, so it cannot provide an exact click-to-first-render latency. RecordingThe complete recording is stored at: Artifact details:
Loading the recordingOpen the
Verify the artifact before comparing results: shasum -a 256 docs/code-howtos/snapshots/entry-editor-first-open-before-lazy-loading.jfrReproduction scenarioThe final capture used a known repository test library and selected a known citation key through the CLI before opening the editor.
The application was launched with: ./gradlew --no-daemon :jabgui:run \
--args="/absolute/path/to/jabref/jablib/src/test/resources/org/jabref/bibtexFiles/test.bib --jumpToKey=1102917" \
--console=plainThe The recording was dumped after the entry editor had opened: jcmd <pid> JFR.dump name=1 filename=entry-editor-first-open-before-lazy-loading.jfrSupplemental linked-file runA second cold-start run used a repository fixture whose six entries each link a local PDF. The first entry,
The application was launched with: JDK_JAVA_OPTIONS='-XX:StartFlightRecording=name=entry,settings=profile,disk=true,maxsize=250m -XX:FlightRecorderOptions=stackdepth=256' \
./gradlew :jabgui:run \
--args="/absolute/path/to/jabref/jablib/src/test/resources/org/jabref/logic/search/test-library-with-attached-files.bib"The recording was dumped while JabRef was still running: jcmd <pid> JFR.dump name=entry filename=/tmp/jabref-entry-editor-linked-cold.jfrThe supplemental recording was analyzed locally and is not part of the committed snapshots. The original recording above remains the reproducible artifact attached to this report. AppleScript interaction probeThe following accessibility script was used while attempting to automate the double-click and detect the editor toolbar: tell application "System Events"
tell process "java"
set frontmost to true
click at {600, 300}
delay 0.05
click at {600, 300}
repeat 400 times
if (count of (UI elements of front window whose role is "AXToolbar")) > 1 then
return "editor-visible"
end if
delay 0.025
end repeat
return "timeout"
end tell
end tellIt was invoked as follows: osascript <<'APPLESCRIPT'
tell application "System Events"
tell process "java"
set frontmost to true
click at {600, 300}
delay 0.05
click at {600, 300}
repeat 400 times
if (count of (UI elements of front window whose role is "AXToolbar")) > 1 then
return "editor-visible"
end if
delay 0.025
end repeat
return "timeout"
end tell
end tell
APPLESCRIPTThis script did not produce a reliable JavaFX double-click. It selected the entry cell in the main table, and the entry editor was subsequently opened by a manual double-click while JFR continued recording. The script's approximately 10.8-second wall-clock result therefore includes the wait for that manual action and is invalid as click-to-render latency. A future automation must emit a verified double-click on the main-table row and record a custom input marker before its timing can be used. Environment
Recording overviewThe most relevant recorded event counts are:
The absence of JavaFX input and pulse-phase events is an important limitation: the snapshot is suitable for locating CPU and allocation work, but not for computing an exact input-to-render latency. Interaction timelineThe timestamps below are shown in Europe/Berlin local time. Thread ID 48 is named
The 2.033-second confirmed evidence window is the difference between the first direct Supplemental interaction evidenceThe manual double-click in the linked-file run was followed by JavaFX application-thread work from approximately 18:02:08.515 until layout settled around 18:02:10.280. This approximately 1.765-second interval is another statistical evidence window, not exact input-to-render latency, because the recording has no custom input marker. During that interval, the JavaFX application thread recorded:
The allocation samples were dominated by JavaFX CSS state and matching structures, including The absence of recorded UI-thread file reads does not prove that no filesystem metadata check occurred, but the profile contains no evidence that resolving or reading an attached PDF caused this stall. The dominant sampled work remains field-control creation, CSS, and layout. ControlsFX-decoration startup experimentControlsFX silently installs its internal A JFR run then launched the linked-file fixture after the entry editor had been open in the preceding session. The editor was constructed during startup without an input command: No This is deliberately not reported as a speedup: the recording overlaps application and library startup, uses editor restoration rather than a verified table interaction, and has no click-to-render marker. The remaining startup CSS and layout activity also shows that eager field-editor construction remains material work. A controlled post-change capture with the original library, entry selection, and an input marker is still required for a direct before/after latency comparison. Relevant code pathThe direct application frame in the execution sample is:
The entry editor also constructs all configured tabs in The current main tab also eagerly creates controls that are not immediately used:
The release tag CSS looked-up-color failuresThe supplemental profile and terminal output show repeated JavaFX CSS conversion failures during entry-editor construction. OpenJFX pull request #2225 fixes looked-up colors that fail when used for There is also a direct entry-editor path to ControlsFX decoration: This makes ControlsFX validation decoration a plausible trigger for the looked-up-color issue when many editors are created together. OpenJFX #2225 is the likely JavaFX-side fix, but an A/B run with that patch, or with validation decoration disabled, is still needed to prove how much of the first-open latency and exception burst it removes. Garbage collectionThe GC events overlapping the end of the interaction were:
The 61.9-millisecond old-collection duration includes concurrent work. Its longest application pause was 8.41 milliseconds. GC therefore cannot account for the observed multi-second JavaFX work window. Other concurrent workThe recording contains network, TLS, cryptography, and JSON samples on Best-supported improvementCreate field editors incrementally instead of constructing the complete field-control tree during the first bind. A safe implementation should:
Lazy tab creation is a secondary opportunity, especially for tabs that are configured but not selected. It does not replace incremental creation in the initially selected fields tab. Separately, test a JavaFX build containing OpenJFX #2225 and compare the CSS exception count and JavaFX application-thread work window. Prewarming the editor may hide the cold-path delay, but reducing the initial scene graph and removing the CSS failure path address the recorded work directly. Validation planRetain this recording as the before snapshot and capture an equivalent after snapshot with the same library, citation key, JVM, and JFR settings. The target outcomes are:
A JMH benchmark can measure isolated field-editor creation and binding, including cold and warm cases. It cannot fully measure JavaFX pulse, CSS, and rendering latency by itself. An automated JavaFX integration measurement should therefore complement JMH for the end-to-end open-editor latency. |
|
The build of this PR is available at https://builds.jabref.org/pull-16467/. |
|
this can be merged now |
github-actions
Bot
added
the
status: to-be-merged
|
You could instantiate an instance of the DecorationPane via Reflection for example (or --add-export the package). It's a bit ugly, but you fix the performance issue. Other similar validation libraries like validatorfx that also use this Pane approach at least allow for a normal instantiation as that should be exported ideally |
|
@crschnick Yeah, I think I will do this in a follow-up and also we will try to continue with the validatorfx |
4 participants
Summary
Profiles the first entry-editor freeze, defers unused collapsed-section controls, and preinstalls ControlsFX decoration at GUI startup. The restored-startup profile avoids the previous color-conversion exception burst, but is not a direct latency comparison.
Analogies: The recording preserves evidence like honey, separates work like chocolate squares, and provides a fixed comparison point like the moon.
jabref-contrib-policy:4.2:reviewed:okSteps to test
./gradlew :jabgui:compileJava :jabgui:checkstyleMain.docs/code-howtos/entry-editor-first-open-performance.mdand its attached JFR baseline.Related issues and pull requests
Relates to #16343
AI usage
OpenAI Codex (model GPT-5). The changes were reviewed, understood, and are owned by the contributor.
AI CHECKLIST.md walkthrough
Code checklist
1. Code self-review
Nullability and control flow
== null/!= nullchecks — JSpecify annotations (@NullMarked,@Nullable,@NonNull) used instead.Objects.requireNonNull(...)— nullability expressed via JSpecify annotations.@NullMarked(org.jspecify.annotations.NullMarked).Optionalconsumed withifPresent/ifPresentOrElse/map/orElseThrow— neverorElse(unusedValue)nor anisPresent()+get()block.StringUtil.isBlank(...)used instead ofs == null || s.isBlank().Exceptions
catch (Exception e)— only specific exceptions are caught.throw new RuntimeException(...)/IllegalStateException(...)— these tear down the whole application.Style and idioms
BibEntryobjects built with withers (withField, notsetField).Pattern.compile(...)constant, notString.matches(...).BackgroundTask, notnew Thread().User-facing text
!, and no labels ending with:.Security
text/htmlresponse.Tests
org.jabref.modelororg.jabref.logicbehavior changed.2. Verification commands
./gradlew :jablib:check— nojablibcode changed../gradlew :jabgui:compileJava :jabgui:checkstyleMain../gradlew modernizer../gradlew --no-configuration-cache :rewriteDryRun../gradlew javadoc.npx markdownlint-cli2for the two changed Markdown files../gradlew :jabgui:testwas attempted but TestFX stalled in JavaFX-event teardown before producing test results.3. Documentation
CHANGELOG.mdentry — no visible user-facing change.docs/requirements/entry-editor.md.4. Pull request
gh pr create --body-file.CHANGELOG.mdTODO placeholder.Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)