Move Splash initialization to EDT and make it no longer modal#9303
Move Splash initialization to EDT and make it no longer modal#9303mbien merged 1 commit intoapache:masterfrom
Conversation
mbien
added
Code cleanup
|
+1 to moving the Swing initialization into the right place. From what I recall, the JVM Since that PR we really should have some code in I assume by "no longer modal" you mean the window type? What changes does that introduce across OS? Curious why it was felt necessary originally? |
|
if the modal change is controversial I can revert it. This is the first of a series of startup optimization PRs - I started with the simple changes. The modality issue was just a drive by change since I had to test it anyway but not the goal. edit: one aspect where this is useful though is while debugging. If you have a breakpoint somewhere in the initialization phase the modal splash is often in the way when wanting to see the debugger. |
|
No issue with the modal revert in my opinion, even if it does bring that old issue back in fixing another one. I'd still like @eirikbakke opinion as he made the changes in #1246 I'm also curious looking at the docs for SplashScreen to revisit why using that support was not enough for hi-dpi. Once that's resolved I'll see if I can fix that WM_CLASS issue in here too, as the workaround in the installers is a little fragile. |
|
Thanks for this! Reading over the Splash class, it does not seem to be very thread-safe, nor does it document which thread may access each method. My impression is that the Splash class is externally accessed from the "main" thread, i.e. thread on which main(String args[]) was originally called. Is that right? For example, SplashComponent (comp) is constructed on the main thread, though it really should be constructed on the EDT. After this PR, it is added to the Frame (frame) on the EDT. In another example, SplashPainter.barLength is updated on the main thread and read on the EDT, without synchronization or "volatile" anywhere. Perhaps we'd need to clean up the rest of the threading issues in this class, before we can safely move any of the code between threads? It's possible that some of this worked only by accident before. On the modality thing: Sure, sounds good to make it non-modal. I tested it on MacOS and didn't see any problems with it. On the USE_LAUNCHER_SPLASH = false thing: I could not get this working with HiDPI on Windows in the past. It's also quite a lot of complexity spent on getting the splash screen to show just a tiny bit earlier in the startup cycle. I don't think it's worth trying to reintroduce it. (On the contrary, it might be a good cleanup to just get rid of the related code, e.g. the part that stores the png of the splash screen to make it available for the splash parameter on next startup etc.) On WM_CLASS: It's good to have this set properly, yes! Is your concern that it is not being set on the splash screen frame, only on the eventual main window? |
overlooked that one. will try to move that somewhere else too after I entangled the spaghetti code I just realized that |
|
this is a mess. The test only worked because it completed before the EDT ran. netbeans/platform/core.startup/src/org/netbeans/core/startup/Splash.java Lines 448 to 450 in 26a05a6 |
mbien
force-pushed
the
edt-splash
branch
2 times, most recently
from
99edbab to
e96a034
Compare
|
Tried to make this work without rewriting everything. Also switched to JFrame too so that the progress bar doesn't flicker on update. |
Well, it also has the benefit of not having
The problem is not that it isn't set, but that it's set to the default. If you start up the toolkit in the main thread, you'll get the name of the main class, but off another thread like we are, the splash gets This started being a problem when we stopped using the JDK splash. And why we get duplicate icons in Linux docks if relying solely on matching against |
Oh, interesting. So the JVM splash becomes a possible workaround. There seems to be two options:
Might be good for a separate PR after this one is merged, since I suspect the threading and cleanup stuff will make this one complex enough already. (EDIT Also, is it so that not having WM_CLASS set on the splash screen also solved the duplicate-taskbar-icon problem on Linux? Then a third option would be to un-set WM_SCALE in Splash.java....) |
|
Yes, let's get @mbien changes in first, and then consider this problem. Let's keep |
|
yep lets try to get the discussion back to this PR. This is primarily about EDT offloading (which is also technically a concurrency bug fix). Secondary is the modality change which can be reverted if there are concerns. Everything else would be followups - which would be appreciated since the code would benefit from more refactoring/cleanups (or a green field rewrite of the splash functionality). |
platform/core.startup/src/org/netbeans/core/startup/Splash.java
Outdated
Show resolved
Hide resolved
| try { | ||
| SplashScreen splashScreen = SplashScreen.getSplashScreen(); | ||
| if (splashScreen != null) { | ||
| painter = new SplashPainter(splashScreen.createGraphics(), null, false); |
There was a problem hiding this comment.
If we are leaving SplashScreen support in, did you test this branch to see that it worked?
There was a problem hiding this comment.
a little bit. I am not sure what the original logic was but what happens is the following:
I added a green version of the splash and passed -J-splash:/home/mbien/splash_green.gif to the dev build of this PR. The green version popped up first, a fraction of a second later the regular NB splash appeared over the first. (the green splash had the wrong size, using the png 1:1, so I did see that it remained behind the regular NB splash which is scaled by the Splash class)
note: when I used --spash, the splash (both of them btw) got centered between my two screens. Without it, it centered on the active screen.
keep in mind this happens really fast. If that is the desired effect, it still works.
There was a problem hiding this comment.
forgot to mention. i tested this only on linux x11. The regular splash I tested on win11 too. (note: i saw the windows platform launcher had splash related code in it - I chose to ignore that and look away)
There was a problem hiding this comment.
when I used --spash, the splash (both of them btw) got centered between my two screens. Without it, it centered on the active screen.
That might be another good reason to just drop "--splash" support.
There was a problem hiding this comment.
The green version popped up first, a fraction of a second later the regular NB splash appeared over the first ... so I did see that it remained behind the regular NB splash
That isn't, or at least shouldn't, be what is happening. If the JDK splash window is there, we should be drawing in to it not opening a separate window.
There was a problem hiding this comment.
That isn't, or at least shouldn't, be what is happening. If the JDK splash window is there, we should be drawing in to it not opening a separate window.
I was wondering about that but I wasn't sure. I believe both approaches would work (replace vs reuse). In any case this code didn't really change anything there as far as i can tell. NB 29 behaves identical when i pass -J-splash:/home/mbien/splash_green.gif to it
|
I looked through the latest version. Splash.java still has various threading issues. For example:
Threading issues have also clearly been an issue in the past, see e.g. the comment "run in AWT, there were problems with accessing font metrics". So if we touch threading at all we might need to do it properly... Some things that might help:
|
this is a different category of threading problems. One tried to initialize awt and swing code on the main thread which is highly problematic. The other might miss out on an update of an integer increment, which is harmless. Worst case is that the progress is wrong by a few pixels. I can give this another pass but won't rewrite the whole thing. Esp not while the requirements are unclear given the launchers and the |
|
I suppose, the alternative to fixing everything "properly" is to just test it for a while. I'd be fine with that, too. |
|
this is now no longer a small change since I extracted the model from the UI code. |
platform/core.startup/test/unit/src/org/netbeans/core/startup/SplashTest.java
Outdated
Show resolved
Hide resolved
|
Looks good and latest version runs fine on MacOS. Did anyone test it on Windows already? Happy to dig up my old windows laptop to test. |
|
Added a PR that should hopefully address the WM_CLASS issue. Should wait and rebase on this. Will try and test this on Windows. I have some concerns about the stated behaviour trying the |
I tested this on linux X11 and win 11 as previously mentioned
I have will do another push with minor changes in a few minutes since I think I can reduce the diff a little bit and squash. |
OK, I checked the JDK Removing the code in here would be more than the comment though - there's lots of Still, happy with how it is now, too. Thanks! |
eirikbakke
left a comment
eirikbakke
left a comment
There was a problem hiding this comment.
Thanks for the edits and cleanup!
|
thanks, will rebase and merge. then continue with the other two startup perf PRs. Cache loading first (#9307, #9308). the gh action queue is full atm. Will wait until CI runs again.
as previously mentioned: already done, here the stash for anyone wanting to do anything with it: Detailsdiff --git a/platform/core.startup/src/org/netbeans/core/startup/Splash.java b/platform/core.startup/src/org/netbeans/core/startup/Splash.java
index 416381f..3c80726 100644
--- a/platform/core.startup/src/org/netbeans/core/startup/Splash.java
+++ b/platform/core.startup/src/org/netbeans/core/startup/Splash.java
@@ -32,15 +32,11 @@
import java.awt.Image;
import java.awt.Rectangle;
import java.awt.RenderingHints;
-import java.awt.SplashScreen;
import java.awt.Toolkit;
import java.awt.Window;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.KeyEvent;
-import java.awt.image.BufferedImage;
-import java.io.DataOutputStream;
-import java.io.IOException;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
@@ -51,7 +47,6 @@
import java.util.StringTokenizer;
import java.util.logging.Level;
import java.util.logging.Logger;
-import javax.imageio.ImageIO;
import javax.swing.Icon;
import javax.swing.JComponent;
import javax.swing.JDialog;
@@ -62,7 +57,6 @@
import javax.swing.SwingConstants;
import javax.swing.SwingUtilities;
import javax.swing.WindowConstants;
-import org.netbeans.Stamps;
import org.netbeans.Util;
import org.openide.util.ImageUtilities;
import org.openide.util.NbBundle;
@@ -74,18 +68,21 @@
/** A class that encapsulates all the splash screen things.
*/
@SuppressWarnings("AccessingNonPublicFieldOfAnotherObject")
-public final class Splash implements Stamps.Updater {
+public final class Splash {
private static volatile Splash splash;
- private volatile SplashPainter painter;
private JFrame frame;
- private SplashComponent comp;
+ private volatile SplashComponent comp;
private final Progress progress;
/** is there progress bar in splash or not */
private static final boolean noBar = Boolean.getBoolean("netbeans.splash.nobar") ||
!Boolean.parseBoolean(NbBundle.getMessage(Splash.class, "SplashShowProgressBar"));
+
+ private Splash() {
+ this.progress = new Progress();
+ }
public static Splash getInstance() {
if (splash != null) {
@@ -115,36 +112,7 @@
ImageUtilities.loadImage("org/netbeans/core/startup/frame1024.png", true)) // NOI18N
);
}
- /**
- * Indicate if we should try to take advantage of java's "-splash" parameter, which allows
- * the splash screen to be displayed at an earlier stage in the app startup sequence. See the
- * original Buzilla RFE at https://netbeans.org/bugzilla/show_bug.cgi?id=60142 . This requires
- * splash screen image(s) to be written to the cache directory the first time NetBeans starts,
- * to be available during subsequent NetBeans startups. Despite
- * https://bugs.openjdk.java.net/browse/JDK-8145173 and
- * https://bugs.openjdk.java.net/browse/JDK-8151787 , as of OpenJDK 10.0.2 and OpenJDK 12.0.1
- * I have found no way to make this work properly with HiDPI screens on Windows. HiDPI filenames
- * attempted include "splash@2x.png", "splash@200pct.png", "splash.scale-200.png", and
- * "splash.java-scale200.png". In all of these cases, the regular "splash.png" file is used
- * instead of one of the 2x-scaled ones (for a system DPI scaling of 200%), and the splash
- * screen becomes half the expected size. Thus, to we disable this feature for now, in favor of
- * a slightly delayed splash screen that appears with the correct size and resolution on HiDPI
- * screens.
- *
- * <p>See also https://issues.apache.org/jira/browse/NETBEANS-67 .
- */
- private static final boolean USE_LAUNCHER_SPLASH = false;
-
- private Splash() {
- this.progress = new Progress();
- Stamps s = Stamps.getModulesJARs();
- if (!CLIOptions.isNoSplash() && !GraphicsEnvironment.isHeadless()) {
- if (USE_LAUNCHER_SPLASH && !s.exists("splash.png")) {
- s.scheduleSave(this, "splash.png", false);
- }
- }
- }
-
+
int getMaxSteps() {
return progress.maxSteps;
}
@@ -162,22 +130,8 @@
}
if (show) {
onEDT(() -> {
- if (painter == null) {
- try {
- SplashScreen splashScreen = SplashScreen.getSplashScreen();
- if (splashScreen != null) {
- painter = new SplashPainter(progress, splashScreen.createGraphics(), null, false);
- }
- } catch (IllegalStateException splashAlreadyClosed) {}
- if (painter == null) {
- comp = new SplashComponent(progress, false);
- painter = comp.painter;
- }
- if (comp == null) {
- return;
- }
- }
if (frame == null) {
+ comp = new SplashComponent(progress, false);
frame = new JFrame(NbBundle.getMessage(Splash.class, "LBL_splash_window_title")); // e.g. for window tray display
initFrameIcons(frame); // again, only for possible window tray display
frame.setUndecorated(true);
@@ -201,6 +155,7 @@
frame.setVisible(false);
frame.dispose();
frame = null;
+ comp = null;
}
});
}
@@ -215,7 +170,7 @@
if (noBar || CLIOptions.isNoSplash()) {
return;
}
- progress.increment(painter, steps);
+ progress.increment(comp != null ? comp.painter : null, steps);
}
public Component getComponent() {
@@ -228,7 +183,7 @@
if (CLIOptions.isNoSplash()) {
return;
}
- progress.setText(painter, text);
+ progress.setText(comp != null ? comp.painter : null, text);
}
/** Adds specified numbers of steps to a progress
@@ -274,15 +229,6 @@
Integer.parseInt(NbBundle.getMessage(Splash.class, "SPLASH_WIDTH")),
Integer.parseInt(NbBundle.getMessage(Splash.class, "SPLASH_HEIGHT")));
}
-
- @Override
- public void flushCaches(DataOutputStream os) throws IOException {
- ImageIO.write((BufferedImage)loadContent(false), "png", os);
- }
-
- @Override
- public void cacheReady() {
- }
private static void onEDT(Runnable edtAction) {
if (SwingUtilities.isEventDispatchThread()) {
@@ -553,12 +499,6 @@
} else {
if (next < System.currentTimeMillis()) {
paint();
- try {
- SplashScreen ss = SplashScreen.getSplashScreen();
- if (ss != null) {
- ss.update();
- }
- } catch (IllegalStateException splashAlreadyClosed) {}
next = System.currentTimeMillis() + 200;
}
} |
- about 100ms faster startup (main method duration measured) and technically more correct anyway - use JFrame to get double buffering (no progress bar flicker during repaint) and we can use the opportunity to initialize swing asynchronously while NB is busy bootstrapping. - other change: splash window is no longer modal - code renovations most changes come from the fact that the model needed to be extracted from UI code so that it can be updated from any thread while the UI paints on EDT.
3 participants
Main#startmethod execution time measured) and technically more correct anywayJFrameto get double buffering (no progress bar flicker during repaint) and we can use the opportunity to initialize swing asynchronously while NB is busy bootstrapping.most changes come from the fact that the model needed to be extracted from UI code so that it can be updated from any thread while the UI paints on EDT.
have to test the dev build on other systems, esp how it interacts with the Java 6
--splashJVM feature (doesn't seem to be used from the linux launcher). I suppose that one would be still modal before its replaced but its hopefully not noticeable. edit: tested it in #9303 (comment)more startup optimizations to come later
closes #8557