Comment 1 Markus Keller 2010-04-08 14:09:14 EDT

The fixes for bug 223315 are very expensive since you now create a lot of ASTs with bindings.

If ToggleBreakpointAdapter.parseCompilationUnit(ITypeRoot, boolean) really needs bindings (I didn't look at the usages at all), then you should at least use the SharedASTProvider as I suggested in bug 223315 comment 12.

I don't have a ready-made solution for ToggleBreakpointAdapter#getQualifiedName(IType), but you need to improve this for 3.6, at least for the normal case (when no local types are involved).

Comment 2 Michael Rennie 2010-04-09 16:42:21 EDT

Created attachment 164432 [details]
wip

This patch is a work in progress which changes the parseCompilationUnit(..) method to use SharedASTProvider. It also takes a stab at changing the getQualifiedName() method to use it as well. I ran into a snag with this change though, it seems the binding key from the IType does not exist in the binding resolver mapping. Consider the following example:

package x.y.z;

public class Test {
  public void method() {
    class LocalClazz {
      public String speak() {
        return "hello"; //set bp here
      }
    }
  }
}

The key I get from IType.getKey() is: Lx/y/z/Test$LocalClazz;

and DefaultBindingResolver contains the following mapping for the local type:

Lx/y/z/Test$68$LocalClazz;=Local type : LocalClazz (id=NoId)
class $Local$LocalClazz
	extends java.lang.Object
	enclosing type : x.y.z.Test
/*   methods   */
void <init>() 
public java.lang.String speak() 

so the lookup for the local type always fails. Markus / Olivier should I be able to look up a local type using its binding key? Or is there a step I am missing?

The code pattern in question:

CompilationUnit unit = SharedASTProvider.getAST(type.getTypeRoot(), SharedASTProvider.WAIT_YES, null);
ASTNode node = unit.findDeclaringNode(type.getKey());

Comment 3 Markus Keller 2010-04-12 10:28:37 EDT

(In reply to comment #2)
> The key I get from IType.getKey() is: Lx/y/z/Test$LocalClazz;

That key looks wrong to me. IType#getKey() does not specify that it doesn't work for unresolved local ITypes (with IType#isResolved() == false).

Olivier, would it be very hard to add an API IType#getBinaryName() (returning the same result as ITypeBinding#getBinaryName())? AFAICS, that would avoid all these dances with resolved ASTs.

Michael, the Javadoc for needsBindings(..) is easy to misunderstand. Since it doesn't deal with type hierarchies, I'd change this to: "Checks if the type or any of its enclosing types are local types."

Comment 4 Olivier Thomann 2010-04-12 10:39:53 EDT

(In reply to comment #3)
> Olivier, would it be very hard to add an API IType#getBinaryName() (returning
> the same result as ITypeBinding#getBinaryName())? AFAICS, that would avoid all
> these dances with resolved ASTs.
The problem is that we don't know the "binary type" as long as we don't generated the corresponding .class file. So I don't think we ever end up creating a binary name for a type that would be in dead code.
Also the binary name is relative to the compliance used to build it. I will see if we can improve it. But the "numbering" of local types or anonymous types through the same unit might be difficult to compute.
I'll take a look anyway.

Comment 5 Michael Rennie 2010-04-12 13:52:51 EDT

Created attachment 164600 [details]
updated  patch

This patch uses the the needsBindings(..) method and visits the AST like we used to - not using the shared AST in getQualifiedName(type). The patch also sets a focal position for the AST if there is one, and includes the doc update Markus suggested.

Comment 6 Michael Rennie 2010-04-12 14:06:01 EDT

Testing the latest patch on StyledText line 9389 (chosen randomly) this once again feels almost instantaneous. Markus, can you try the latest patch to confirm my findings?

Comment 7 Markus Keller 2010-04-12 14:31:13 EDT

(In reply to comment #2)
> Created an attachment (id=164432) [details] [diff]
> wip

I'm sure you'll add
    parent = parent.getParent();
at the end of the 'while' block in ToggleBreakpointAdapter#needsBindings(IType), right? ;-)

Apart from that and the problem with the key for local types, setting breakpoints in the current editor is again blindingly fast with this patch.

The only remaining delay I saw was when setting a breakpoint right after activating an existing editor tab. In that case, there's no reconcile, so the first client asking for an AST gets the full delay. I'll discuss that with Dani.


(In reply to comment #5)
> Created an attachment (id=164600) [details] [diff]
> updated  patch

The focal position is useless for ASTParser#createBindings(..).

I've tested the patch from comment 2 with the fix in the while loop, and that was really quick. The patch from comment 5 is still quite slow when you actually reach the code that creates the parser. Example: Set a line breakpoint in the Runnable inside the constructor of StyledText.

Comment 8 Michael Rennie 2010-04-12 15:02:42 EDT

(In reply to comment #7)
> ...
> ToggleBreakpointAdapter#needsBindings(IType), right? ;-)

of course

> I've tested the patch from comment 2 with the fix in the while loop, and that
> was really quick. The patch from comment 5 is still quite slow when you
> actually reach the code that creates the parser.

The problem with the patch from comment #2 though is that is short circuits to trying to compute the name ourselves (bringing back the original bug) because the bindings are never found. In the interim the last patch at least cuts down creating an AST to only cases where local types are involved.

> Example: Set a line breakpoint in the Runnable inside the constructor of 
> StyledText.

This example does not hit the code for creating the parser

Comment 9 Michael Rennie 2010-04-12 15:04:22 EDT

Created attachment 164617 [details]
wip 3

updated patch investigating using needsBindings(...) + old way of parsing bindings

Comment 10 Michael Rennie 2010-04-12 15:09:24 EDT

(In reply to comment #8)

> Example: Set a line breakpoint in the Runnable inside the constructor of 
> StyledText.

> This example does not hit the code for creating the parser

I was wrong, once I brought the source into my workspace it does hit it, I was trying in the class file editor before.

Comment 11 Michael Rennie 2010-04-12 15:22:49 EDT

Created attachment 164619 [details]
wip 4

even better in the needsBindings(..) method we should add !type.isAnonymous(), since the old way of computing the name correctly handles anonymous type names.

Comment 12 Michael Rennie 2010-04-12 15:30:04 EDT

Created attachment 164621 [details]
wip 4b

Small update for copy / paste problem.

Testing with the following trivial snippet in the StyledText constructor is still very slow, but in the anonymous case it is fast again.

class Local {
  void method() {
    System.out.println();
  }
}

Comment 13 Markus Keller 2010-04-21 10:28:01 EDT

Filed bug 309966 for the problems with IType.getKey() from comment 2 and 3.

Please release "wip 4b". It's only slow for non-anonymous local types, and those are rare in practice.


However, I found a remaining problem where bug 223315 still shows up. Add this local class to the constructor of StyledText:

class Local {
	{
		System.out.println(); //toggle breakpoint not reliable here
	}
	void m() {
		System.out.println("StyledText.StyledText(...).Local.m()");
	}
}

=> When I double-click a second time in the initializer, a second BP is added.

The fix is in ToggleBreakpointAdapter#toggleBreakpoints(..): You should add
    "|| mtype == IJavaElement.INITIALIZER"
to the following if statement:
    "if(mtype == IJavaElement.FIELD || mtype == IJavaElement.METHOD) {..}"

Comment 14 Michael Rennie 2010-04-21 10:52:40 EDT

applied patch with suggested fix for initializers and updated the test plan template to include a section for member types + initializers

Comment 15 Markus Keller 2010-04-22 16:35:04 EDT

Created attachment 165849 [details]
Fix 5

There's a much better solution, see bug 309966 comment 2.

Here's an implementation that never needs to create another AST.


Tested also in bizarre scenarios like this:

import java.util.concurrent.Callable;
class A {
	public static void main(String[] args) {
		class Local {
			{
				System.out.println("A.main(...).Local.main()");
				m();
			}
			void m() {
				System.out.println("A.main(...).Local.m()");
				try {
					new Callable<Integer>() {
						@Override
						public Integer call() throws Exception {
							System.out.println("A.main(...).Local.m().new Callable<Integer>() {...}.call()");
							return null;
						}
					}.call();
				} catch (Exception e) {
					e.printStackTrace();
				}
			}
		}
		System.out.println("A.main()");
		new Local();
    }
}

Comment 16 Markus Keller 2010-04-22 16:36:15 EDT

Reopening for the bestest(TM) solution.

Comment 17 Olivier Thomann 2010-04-22 18:35:59 EDT

Thanks Markus. This is exactly the kind of code I had in mind.

Comment 18 Michael Rennie 2010-04-23 11:51:20 EDT

I like fix 5, it works very well, you barely notice any lag at all, even with large source files.

Comment 19 Darin Wright 2010-04-23 16:08:44 EDT

Applied.