Bug 342735

Comment 1 Paul Webster 2011-04-13 12:49:27 EDT

(In reply to comment #0)
> org.eclipse.ui.navigator,3.5.0.I20100601-0800 
>   The class org/eclipse/ui/navigator/LinkHelperService.class is different
> 

Andrew, do you know what the problem is?  I checked LinkHelperService, for example, and it hasn't changed since Jan 09.

Was this a side-effect of the compiler over-optimization bug that we compiled with for a short time?  Or is it just the new version of the compiler emits different code than the one that compiled 3.6.x ?

PW

Comment 2 Andrew Niefer 2011-04-13 13:09:08 EDT

Created attachment 193175 [details]
disassembled #1

This is the previously published version

Comment 3 Andrew Niefer 2011-04-13 13:10:21 EDT

Created attachment 193176 [details]
disassembled #2

This is the newly built version.

There is a diff in #getLinkHelper(LinkHelperDescriptor)

Comment 4 Andrew Niefer 2011-04-13 13:11:10 EDT

Perhaps Olivier can explain the diff, it seems perhaps to be a difference with the compiler.

Comment 5 Paul Webster 2011-04-13 13:21:57 EDT

I'm not questioning the need, just curious.

PW

Comment 6 Oleg Besedin 2011-04-13 13:54:39 EDT

Interesting. The source code for the method is:

private ILinkHelper getLinkHelper(LinkHelperDescriptor descriptor) {
	ILinkHelper helper = (ILinkHelper) linkHelpers.get(descriptor);
	if (helper == null) {
		synchronized (this) {
			if (helper == null) {
				linkHelpers.put(descriptor, helper = descriptor
						.createLinkHelper());
			}
		}
	}
	return helper;
}

From diffs, the "old" version ignores the second "if (helper == null)" check which seems to be a correct optimization.

Comment 7 Olivier Thomann 2011-04-13 14:02:01 EDT

I don't understand why this was not notified before today. This would be related to the fact that the null analysis no longer has an impact on the code generation. This was changed a while ago.
So the change is safe. I simply wonder why this was not found before.

Comment 8 Olivier Thomann 2011-04-13 14:04:52 EDT

Note that this double null check is actually bogus.
See bug 329226 comment 0.

Comment 9 Andrew Niefer 2011-04-13 14:14:29 EDT

(In reply to comment #7)
> I don't understand why this was not notified before today. This would be
> related to the fact that the null analysis no longer has an impact on the code
> generation. This was changed a while ago.
> So the change is safe. I simply wonder why this was not found before.

I'm not sure that anyone is looking at these comparator logs.  I don't see a link to them from the download page.  The only reason I looked at them today was because of bug 341952 which doesn't seem to be bothering anyone else. (This would be a blocker except that it doesn't happen in 4.1 and I'm using command line git more than CVS these days).

Comment 10 Oleg Besedin 2011-04-13 15:11:08 EDT

I've tagged the following bundles with I20110413-1600 :

org.eclipse.ui.examples.multipageeditor
org.eclipse.ui.examples.propertysheet
org.eclipse.ui.examples.readmetool
org.eclipse.ui.examples.undo
org.eclipse.ui.navigator
org.eclipse.ui.workbench.compatibility

The following two bundles belong to other groups:

org.eclipse.ui.examples.javaeditor	->	Platform/Text
org.eclipse.ui.console	->	Platform/Debug

Comment 11 Oleg Besedin 2011-04-13 15:15:21 EDT

I opened bug 342753 for Platform/Text and bug 342754 for Platform/Debug.

Comment 12 Dani Megert 2011-04-14 01:44:16 EDT

>I'm not sure that anyone is looking at these comparator logs.
Correct, because we rely on the 'testComparatorLogs' releng test which does no longer seem to work: in the I-build it says: "Success". I've file bug 342786 to track this.

Comment 13 Dani Megert 2011-04-20 02:34:43 EDT

Verified in I20110419-1004.