Revert "Postpone long-running operations to the activation of the "Tracing" tab." #925

HeikoKlare · 2023-11-16T16:44:12Z

See eclipse-platform/eclipse.platform#859 and in particular eclipse-platform/eclipse.platform#859 (comment).

This reverts commit 7c1f0ed from #674. The change introduced a regression causing Tracing tabs not to be initialized properly in specific situation. It requires an update to the Eclipse platform, which will not be integrated into the upcoming release anymore. Therefore, the change is reverted for the release to be reapplied after the next release.

The effect of reverting the change will be a loss of the performance improvement in the launch configurations dialog that was gained by the change.

…acing" tab." This reverts commit 7c1f0ed from eclipse-pde#674. The change introduced a regression causing Tracing tabs not to be initialized properly in specific situation. It requires an update to the Eclipse platform, which will not be integrated into the upcoming release anymore. Therefore, the change is reverted for the release to be reapplied after the next release. The effect of reverting the change will be a loss of the performance improvement in the launch configurations dialog that was gained by the change.

iloveeclipse · 2023-11-16T17:01:19Z

@fedejeanne : please describe what exactly happens (from the user point of view) for "Tracing tabs not to be initialized properly in specific situation".

@HannesWell: please asses if we want to have the regression or revert to be in RC2.

github-actions · 2023-11-16T17:18:12Z

Test Results

    270 files ±0     270 suites ±0 46m 48s ⏱️ - 1m 18s
  3 327 tests ±0   3 297 ✔️ ±0 30 💤 ±0 0 ❌ ±0
10 278 runs ±0 10 188 ✔️ ±0 90 💤 ±0 0 ❌ ±0

Results for commit 921f56f. ± Comparison against base commit f169a74.

fedejeanne · 2023-11-16T19:11:03Z

Thank you very much @HeikoKlare for creating this PR!

Sorry for the late response, I was able to sit in front of the computer again just recently.

@fedejeanne : please describe what exactly happens (from the user point of view) for "Tracing tabs not to be initialized properly in specific situation".

@iloveeclipse If the "Tracing" tab is not initialized properly then switching between 2 PDE (Test) run configurations (from "A" to "B") could either:

Loose the tracing options that had been activated previously in "B" i.e. tracing is always deactivated for "B", or
Even throw an exception when switching to "B"

Applying this PR i.e. reverting #674 fixes that regression so this PR is OK from my point of view ✔️

HannesWell · 2023-11-16T20:24:11Z

@HannesWell: please asses if we want to have the regression or revert to be in RC2.

I'm not yet sure if we should revert this or not.
Please correct me if I'm wrong, but the regression is not severe and one can recover from it relativly easy?
On the other hand If we revert this it means that the initializaion will take longer for everyone even if tracing is not used/modified.

Personally I don't use tracing and I assume that in general tracing is not a widely used feature. With that I assume that more would benefit from the performance improvement than being hit by the regression.

Do you think there is a simple fix we could apply in PDE to at least handle the exception?

fedejeanne · 2023-11-17T07:52:55Z

Please correct me if I'm wrong, but the regression is not severe and one can recover from it relativly easy?

Well it all depends on whether or not you use tracing. I don't, in fact I hadn't even opened the tab before performing the fix. The consequences of the regression are that you loose the whole information you saved before and you need to: (EDITED)

Click away the error message
Select another tab and then the Tracing tab in order to "activate" it
"Enable tracing" again and check everything you want to trace.

Do you think there is a simple fix we could apply in PDE to at least handle the exception?

I looked into it and I couldn't find an easy way that affects only the Tracing tab :-\

fedejeanne · 2023-11-17T07:57:59Z

FWIW when I said:

Applying this PR i.e. reverting #674 fixes that regression so this PR is OK from my point of view ✔️

I didn't mean it like "I'd like to have my previous fix reverted", I just meant it like "Applying this PR reverts the regression without any new side effects".

I'm with @HannesWell on this one: I can live with the regression but I can't stand waiting 10-20 seconds every now and then when I open the "Run Configurations" dialog again after a few (Eclipse) restarts.

laeubi · 2023-11-17T08:20:55Z

Is there a Stacktrace or code location where this happens? It seems quite trivial to check for null and either use false/true (whatever seems sufficient) instead... just guessing from the error message!

fedejeanne · 2023-11-17T08:58:29Z

Is there a Stacktrace or code location where this happens? It seems quite trivial to check for null and either use false/true (whatever seems sufficient) instead... just guessing from the error message!

I tried that but it only prevents the error dialog from showing up, nothing else. The configurations get lost anyway i.e. the user has to click "Enable tracing" and select the plugins again.

I also tried activating the Tracing tab (actually, the TracingBlock) inside TracingBlock::pluginSelected (see stack-trace below) so all the data would be there: no luck, the data gets lost and the user has to enable the tracing and configure it again.

Here's the stack-trace.

java.lang.NullPointerException: Cannot invoke "java.lang.Boolean.booleanValue()" because the return value of "java.util.Map.get(Object)" is null
	at org.eclipse.pde.internal.ui.launcher.TracingPropertySource$BooleanEditor.initialize(TracingPropertySource.java:157)
	at org.eclipse.pde.internal.ui.launcher.TracingPropertySource.createContents(TracingPropertySource.java:234)
	at org.eclipse.pde.internal.ui.launcher.TracingBlock.pluginSelected(TracingBlock.java:444)
	at org.eclipse.pde.internal.ui.launcher.TracingBlock.lambda$2(TracingBlock.java:188)
	at org.eclipse.jface.viewers.CheckboxTableViewer$1.run(CheckboxTableViewer.java:216)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:47)
	at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:174)
	at org.eclipse.jface.viewers.CheckboxTableViewer.fireCheckStateChanged(CheckboxTableViewer.java:213)
	at org.eclipse.jface.viewers.CheckboxTableViewer.handleSelect(CheckboxTableViewer.java:304)
	at org.eclipse.jface.viewers.StructuredViewer$4.widgetSelected(StructuredViewer.java:1204)
	at org.eclipse.jface.util.OpenStrategy.fireSelectionEvent(OpenStrategy.java:263)
	at org.eclipse.jface.util.OpenStrategy$1.handleEvent(OpenStrategy.java:421)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4273)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1066)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4071)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3659)
	at org.eclipse.jface.window.Window.runEventLoop(Window.java:823)
	at org.eclipse.jface.window.Window.open(Window.java:799)
	at org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationsDialog.open(LaunchConfigurationsDialog.java:1240)
	at org.eclipse.debug.ui.DebugUITools.lambda$1(DebugUITools.java:630)
	at org.eclipse.swt.custom.BusyIndicator.showWhile(BusyIndicator.java:67)
	at org.eclipse.debug.ui.DebugUITools.openLaunchConfigurationDialogOnGroup(DebugUITools.java:636)
	at org.eclipse.debug.ui.DebugUITools.openLaunchConfigurationDialogOnGroup(DebugUITools.java:574)
	at org.eclipse.debug.ui.actions.OpenLaunchDialogAction.run(OpenLaunchDialogAction.java:85)
	at org.eclipse.jface.action.Action.runWithEvent(Action.java:474)
	at org.eclipse.jface.action.ActionContributionItem.handleWidgetSelection(ActionContributionItem.java:580)
	at org.eclipse.jface.action.ActionContributionItem.lambda$4(ActionContributionItem.java:414)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4273)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1066)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4071)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3659)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1155)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:342)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1046)
	at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:155)
	at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:645)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:342)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:552)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:173)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:152)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:208)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:136)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:402)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:255)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:651)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:588)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1459)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1432)

iloveeclipse · 2023-11-17T09:09:16Z

I personally use "tracing" often and so far I had no issues with performance of it.
However, I will have an issue if "tracing" page contains wrong / unexpected values.

That said, the performance issue exists since ever, and I think the risk of introducing other regression by "quick fixing" current error is much higher as to revert the change, which should just restore the status quo.

So I would propose to revert the change that breaks tracing page.

laeubi · 2023-11-17T09:34:28Z

@fedejeanne just some quick codereview and the following observations
in org.eclipse.pde.internal.ui.launcher.TracingPropertySource.createContents(Composite, boolean) there is the editor created, but the map only will contain a value if the master value is set.

Should it here

eclipse.pde/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/launcher/TracingPropertySource.java

Lines 223 to 225 in f169a74

 if (masterValue != null) { 

 fValues.put(shortKey, Boolean.valueOf(masterValue)); 

 }

not be something like that:

if (masterValue != null) {
	fValues.put(shortKey, Boolean.valueOf(masterValue));
} else {
	fValues.put(shortKey, Boolean.valueOf(value));
}

if the fValues is only meant to track the (inital) master value (what it not clear to me) I would expect:

eclipse.pde/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/launcher/TracingPropertySource.java

Lines 156 to 160 in f169a74

 public void initialize() { 

 boolean value = (Boolean) fValues.get(getKey()); 

 checkbox.setSelection(value); 

 checkbox.addSelectionListener(widgetSelectedAdapter(e -> valueModified(checkbox.getSelection()))); 

 valueModified(value);

to look like this:

public void initialize() {
	if ( fValues.get(getKey()) instanceof Boolean value) {
		checkbox.setSelection(value);
		valueModified(value);
	}
	checkbox.addSelectionListener(widgetSelectedAdapter(e -> valueModified(checkbox.getSelection())));
}

fedejeanne · 2023-11-17T10:11:49Z

@laeubi thank you for the input. I tested it and it's the same: the options get lost when moving between configurations.

So I'm curious about how to go on from here: it seems like we all have already "made our cases" and the implications of applying/not applying this PR are well known by now so...

... do we need to vote?
... or does anyone make the decision? Who?

iloveeclipse · 2023-11-17T10:16:09Z

... or does anyone make the decision? Who?

PDE project lead / PMC.

@merks , @HannesWell : please take over.

merks · 2023-11-17T10:21:31Z

@iloveeclipse You have PMC +1 approval for whatever the team lead decides based on a careful analysis of all the considerations.

HannesWell · 2023-11-17T20:06:37Z

So I'm curious about how to go on from here: it seems like we all have already "made our cases" and the implications of applying/not applying this PR are well known by now so...

I'll looking into/reproduce this in detail tomorrow (its too late for today), but from the updated descriptions it sounds like the regression is more severe than initially tought.
And in the end correctness trumps performance, unless the incorrectness is just minor and more an inconvenience.

I'm sorry @fedejeanne but this will probably mean that this reversion has to be applied. But I'll make the final decision tomorrow.

HannesWell · 2023-11-18T20:07:46Z

@iloveeclipse If the "Tracing" tab is not initialized properly then switching between 2 PDE (Test) run configurations (from "A" to "B") could either:
* Loose the _tracing options_ that had been activated previously in "B" _i.e._ tracing is always deactivated for "B", or

* Even throw an exception when switching to "B"

I have reproduced these cases in detail and besides loosing the choices just made in another config, even worse the described issue also made existing launch configs created and successfully saved in a previous session to loose their tracing configuration.
This is a sever regression for users of the tracing options (even if not many use it, but it is used as Andrey said).

I'm very sorry @fedejeanne but with that knowledge we have to apply this reversion of #674.
Nevertheless thank you for reporting this regression. Everyone likes new features and performance improvements and they are exciting, but at the same time existing useful/used features must continue to work, otherwise it becomes frustrating.
So keep them working is as important as implementing new features or improvements (although the latter is often more interesting).

I suggest to re-created #674 once eclipse-platform/eclipse.platform#860 is merged when the master reopens. You can create a corresponding PR now already.

fedejeanne · 2023-11-21T05:23:09Z

@HannesWell no worries, it happens. Thank you for testing it and reporting back.

We can restore the functionality and apply the fixes once the master branch is reopened 👍

vogella · 2023-11-22T15:18:31Z

We can restore the functionality and apply the fixes once the master branch is reopened 👍

Looking forward to the re-application of the fix. At one of my clients opening a runtime configuration takes around 30 seconds and the freeze monitor points always to the tracing tab.

fedejeanne · 2023-11-29T09:08:32Z

We can restore the functionality and apply the fixes once the master branch is reopened 👍

Looking forward to the re-application of the fix. At one of my clients opening a runtime configuration takes around 30 seconds and the freeze monitor points always to the tracing tab.

--> #946

HeikoKlare mentioned this pull request Nov 16, 2023

The first tab is not properly activated when opening the "Run configurations" dialog eclipse-platform/eclipse.platform#859

Closed

HeikoKlare marked this pull request as ready for review November 16, 2023 17:26

HannesWell merged commit eeffb96 into eclipse-pde:master Nov 18, 2023
15 of 16 checks passed

HeikoKlare deleted the revert-674 branch November 20, 2023 06:59

fedejeanne mentioned this pull request Nov 29, 2023

Recreate #674: Postpone long-running operations to the activation of the "Tracing" tab. #946

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "Postpone long-running operations to the activation of the "Tracing" tab." #925

Revert "Postpone long-running operations to the activation of the "Tracing" tab." #925

HeikoKlare commented Nov 16, 2023

iloveeclipse commented Nov 16, 2023

github-actions bot commented Nov 16, 2023

fedejeanne commented Nov 16, 2023

HannesWell commented Nov 16, 2023

fedejeanne commented Nov 17, 2023 •

edited

Loading

fedejeanne commented Nov 17, 2023 •

edited

Loading

laeubi commented Nov 17, 2023

fedejeanne commented Nov 17, 2023

iloveeclipse commented Nov 17, 2023

laeubi commented Nov 17, 2023 •

edited

Loading

fedejeanne commented Nov 17, 2023

iloveeclipse commented Nov 17, 2023

merks commented Nov 17, 2023

HannesWell commented Nov 17, 2023

HannesWell commented Nov 18, 2023 •

edited

Loading

fedejeanne commented Nov 21, 2023

vogella commented Nov 22, 2023

fedejeanne commented Nov 29, 2023

Revert "Postpone long-running operations to the activation of the "Tracing" tab." #925

Revert "Postpone long-running operations to the activation of the "Tracing" tab." #925

Conversation

HeikoKlare commented Nov 16, 2023

iloveeclipse commented Nov 16, 2023

github-actions bot commented Nov 16, 2023

Test Results

fedejeanne commented Nov 16, 2023

HannesWell commented Nov 16, 2023

fedejeanne commented Nov 17, 2023 • edited Loading

fedejeanne commented Nov 17, 2023 • edited Loading

laeubi commented Nov 17, 2023

fedejeanne commented Nov 17, 2023

iloveeclipse commented Nov 17, 2023

laeubi commented Nov 17, 2023 • edited Loading

fedejeanne commented Nov 17, 2023

iloveeclipse commented Nov 17, 2023

merks commented Nov 17, 2023

HannesWell commented Nov 17, 2023

HannesWell commented Nov 18, 2023 • edited Loading

fedejeanne commented Nov 21, 2023

vogella commented Nov 22, 2023

fedejeanne commented Nov 29, 2023

fedejeanne commented Nov 17, 2023 •

edited

Loading

fedejeanne commented Nov 17, 2023 •

edited

Loading

laeubi commented Nov 17, 2023 •

edited

Loading

HannesWell commented Nov 18, 2023 •

edited

Loading