Bug 578077

Comment 1 Jörg Kubitz 2022-01-06 06:00:35 EST

Created attachment 287786 [details]
Example swt events - grouping.png

Comment 2 Jörg Kubitz 2022-01-06 06:01:04 EST

Created attachment 287787 [details]
Example swt events - IDE startup.png

Comment 3 Niraj Modi 2022-01-10 10:00:03 EST

Some interesting work here, below gerrit link missing on this buzilla:
https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/189308

Thanks Jörg for the patch, we will review this shortly.

Comment 4 Eclipse Genie 2022-01-11 16:38:48 EST

New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/189491

Comment 5 Alexandr Miloslavskiy 2022-01-11 16:41:50 EST

I tried it, but unfortunately it crashes for me.

I'm using snippet from the other Gerrit I have just submitted:
https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/189491

By the way, the snippet has detailed instructions on how to try it (up to the crash).

I'm using OpenJDK 11.0.8 on Win11.

I'm using this commandline to start snippet:
java.exe -XX:StartFlightRecording:filename=dump.jfr,dumponexit=true -Dswt.library.path=<...>/org.eclipse.swt.win32.win32.x86_64 -classpath "<...>" org.eclipse.swt.tests.manual.Bug578077_JfrDisplayEvents

I reliably receive this crash as soon as I press a button in the snippet:
----
Exception in thread "main" java.lang.LinkageError: loader 'app' attempted duplicate class definition for org.eclipse.swt.widgets.JfrDisplayEvent$JfrDisplayAsyncExecEvent. (org.eclipse.swt.widgets.JfrDisplayEvent$JfrDisplayAsyncExecEvent is in unnamed module of loader 'app')
  at java.base/java.lang.ClassLoader.defineClass1(Native Method)
  at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1017)
  at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:174)
  at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:800)
  at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:698)
  at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:621)
  at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:579)
  at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
  at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
  at org.eclipse.swt.widgets.Synchronizer.syncExec(Synchronizer.java:219)
  at org.eclipse.swt.widgets.Display.syncExec(Display.java:4780)
  at org.eclipse.swt.tests.manual.Bug578077_JfrDisplayEvents.lambda$main$1(Bug578077_JfrDisplayEvents.java:42)
  at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89)
  at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4244)
  at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1060)
  at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4061)
  at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3633)
  at org.eclipse.swt.tests.manual.Bug578077_JfrDisplayEvents.main(Bug578077_JfrDisplayEvents.java:69)
----

Comment 6 Andrey Loskutov 2022-01-11 17:33:00 EST

(In reply to Alexandr Miloslavskiy from comment #5)
> I reliably receive this crash as soon as I press a button in the snippet:
> ----
> Exception in thread "main" java.lang.LinkageError: loader 'app' attempted
> duplicate class definition for
> org.eclipse.swt.widgets.JfrDisplayEvent$JfrDisplayAsyncExecEvent.
> (org.eclipse.swt.widgets.JfrDisplayEvent$JfrDisplayAsyncExecEvent is in
> unnamed module of loader 'app')
>   at java.base/java.lang.ClassLoader.defineClass1(Native Method)
>   at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1017)

Interesting. So the JFR tries to redefine and reload classes in a *hope* they aren't used anymore, or tries to load them in parallel? This is a critical JFR bug. Have you tried with Java 17? I wonder if they have fixed it meanwhile? That is for sure not the first app that crashes JFR, if small SWT snippet is enough?

Something like https://bugs.openjdk.java.net/browse/JDK-8252904 but for Java 11?
or https://bugs.openjdk.java.net/browse/JDK-8249009 ? 

Honestly, that disqualified JFR now for me. Because it would mean, with a bit more "exotic" GC/runtime configuration or just on different hardware (if that is timing issue) one may run into random crashes just because of tracing code... Like use a different GC (that unloads classes differently or doesn't unload at all), different number / config of GC threads, different optimizations enabled / disabled etc.

Sure, no software without bugs, but adding that extra layer of uncertainty into the basic Eclipse code is unjustifiable.

Comment 7 Jörg Kubitz 2022-01-11 17:33:49 EST

i forwarded that error to https://bugreport.java.com/bugreport:
"Getting an "duplicate class definition" error when having a static method that returns a super type of the the surrounding type. The method does not need to be called."
minimal example:

class AbstractJfrEvent extends jdk.jfr.Event {

	static MyJfrEvent createEvent() {
		return null;
	}

	public static void main(String[] args) {
		new MyJfrEvent();
	}
}

class MyJfrEvent extends AbstractJfrEvent {
};

Comment 8 Alexandr Miloslavskiy 2022-01-11 17:39:54 EST

> Have you tried with Java 17?

Being a terribly lazy guy, I'll let Joerg answer these questions :)

Comment 9 Jörg Kubitz 2022-01-12 01:51:40 EST

it's now
https://bugs.openjdk.java.net/browse/JDK-8279902

Comment 10 Jörg Kubitz 2022-01-12 10:37:00 EST

(In reply to Andrey Loskutov from comment #6)
> it would mean, with a bit more "exotic" GC/runtime configuration 

The bug is pretty deterministic and happens on all JVMs - but only if recording is started and not with osgi class loader. Strange that my code was the first to show it up. Since it's obvious to work around and to test i do not fear further errors. We can not exclude JFR anyways since there are already Events build-in in the JVM. You run it even if your code does not know about. 

I don't want to push the usage of JFR, this ticket is a question. It does not make much sense if only i would use those events. But please give it a try. You may like it once getting familiar with it. 
For those who do not need it they can just leave Flight recording disabled => no problems.

Comment 11 Alexandr Miloslavskiy 2022-01-16 21:44:31 EST

I have submitted updated test snippet that details all steps to try JFR with Mission Control.

Now that I tried JFR+MissionControl, I think that I'm not going to use it.

I understand that the pros of JFR and Mission Control are:
* It records various kinds of events out of the box
* Recording can be started programmatically from the application itself

None of the pros apply to my use cases.

However, the cons (for me) are:
* A dedicated profiler, such as JProfiler, is much more convenient
* Events written to text log are easier to manage (unprepared users can already understand text logs, easier to quote in bug reports)
* I have no idea how to de-obfuscate dump (captured on commercial obfuscated product such one I'm working on)

All of these cons are important enough to me.

As for the events added in this patch, these won't help me either:
* Their value is low
  * For `display.syncExec()`, caller is already on stack
  * For `display.asyncExec()`, it's not too hard to just walk code references to find caller
* I need to make another recording and use a separate tool to view JFR events

Comment 12 Jörg Kubitz 2022-02-02 04:51:50 EST

I still think those events make technical sense. But it does not make sense to add them to the code when nobody beside me will use them.

=> wontfix.