Looking up the stack frame's associated source file from source containers directly to improve searching perf and bugs by testforstephen · Pull Request #127 · microsoft/java-debug

testforstephen · 2017-12-07T09:45:45Z

See the feature description microsoft/vscode-java-debug#121

Previous debugger use jdt search engine to look up the associated source file for the stack frame, but it has several issues:

Search Engine will resolve classes that is not in current project among workspace which makes the source mapping not compatible with project settings. (Should avoid to use SearchEngine in jdt which is not compatible with project settings. #31)
Search Engine cannot handle the class name with $ very well. (Should support class/package name with special characters. vscode-java-debug#19)
The Search policy is a kind of range search, and the result is not accurate. It could get multiple similar results for the search key so that it shows the wrong source for the stack trace. (The debugger jump to the wrong source code for Step.java vscode-java-debug#101)
The jdt search engine is based on index, for big project, it will take a very long time (even minutes) to construct the index file. So when import a new project in VSCode and start to debug it, the first stack frame call would take a little long time. But if the index is built, the search engine perf is acceptable.

…ck frame

…ourcelookup

akaroml · 2017-12-07T15:31:08Z

@@ -164,17 +170,32 @@ public String[] getFullyQualifiedName(String uri, int[] lines, int[] columns) th

    @Override
    public String getSourceFileURI(String fullyQualifiedName, String sourcePath) {


Seems like the fullyQualifiedName is never used. Shall we just delete it?

Different provider implementation may have different source lookup mechanism. I prefer to keep current interface design.
If there is user compliant, i'd like to change the interface to public String getSourceFileURI(StackFrame stackFrame).
From the stack frame, the provider could get more info for accurate searching, such as class name, source name, source path, even source info for the specified stratum (like scala).

akaroml · 2017-12-07T15:33:04Z

-        // But because the inner class and anonymous class have the same java file as the enclosing type,
-        // search their enclosing type instead.
-        if (fullyQualifiedName.indexOf("$") >= 0) {
-            return searchDeclarationFileByFqn(AdapterUtils.parseEnclosingType(fullyQualifiedName));


Please delete the obsolete method searchDeclarationFileByFqn and searchDeclarationFileByFqn. And even AdapterUtils.parseEnclosingType.

akaroml · 2017-12-07T15:55:38Z

+     *                  the current project name.
+     * @return the possible source container list.
+     */
+    public static ISourceContainer[] getSourceContainers(String currentProject) {


A static function is typically stateless. So it usually does not have the knowledge of the "current" project. Suggest using the more stateless name "projectName".

akaroml · 2017-12-07T16:00:06Z

+
+        List<IProject> workspaceProjects = Arrays.asList(ResourcesPlugin.getWorkspace().getRoot().getProjects());
+        projects.addAll(workspaceProjects.stream().filter((project) -> {
+            return !project.equals(targetProject);


Maybe we can just use whatever returned by getProjects() instead of filtering out the targetProject. The logic seems redundant. The project collection should already have the target project. So this function can have no parameter.

The original purpose is "If the project name is specified, it will put the source containers from the specified project in the front of the result. "

The filter logic is to remove duplicated project to avoid duplicated computation.

Yes, it's unnecessary, because the final source container result is a set object, it'll remove the duplicated source container. Will change it as follows.

IProject targetProject = JdtUtils.getProject(projectName); if (targetProject != null) { projects.add(targetProject); } List workspaceProjects = Arrays.asList(ResourcesPlugin.getWorkspace().getRoot().getProjects()); projects.addAll(workspaceProjects);

akaroml · 2017-12-07T16:03:42Z

+    }
+
+    private static ISourceContainer[] getSourceContainers(IJavaProject project, Set<IRuntimeClasspathEntry> calculated) {
+        if (project != null && project.exists()) {


Use early return.

if (project == null || !project.exists()) {
return empty;
}

In this way we avoid indenting large chunk of code.

akaroml · 2017-12-08T05:49:11Z

-        projects.addAll(workspaceProjects.stream().filter((project) -> {
-            return !project.equals(targetProject);
-        }).collect(Collectors.toList()));
+        projects.addAll(workspaceProjects);


There can be duplicated entries inside the projects collection. The targetProject is already inside.

ResourcesPlugin.getWorkspace().getRoot().getProjects() JdtUtils.getProject(projectName)

Does the return value of the first call contain the return value of the second call?

YES, it contains the second result.

But it's OK to have duplicated projects because the later computation code has logic to avoid repeated calculation on the duplicated classpath entries.

yaohaizh · 2017-12-08T09:24:23Z

+        List<IProject> workspaceProjects = Arrays.asList(ResourcesPlugin.getWorkspace().getRoot().getProjects());
+        projects.addAll(workspaceProjects);
+
+        Set<IRuntimeClasspathEntry> calculated = new LinkedHashSet<>();


Use Java.Stream to optimize the coding logic below

nice. fixed.

akaroml · 2017-12-11T07:01:47Z

+
+        Set<IRuntimeClasspathEntry> calculated = new LinkedHashSet<>();
+
+        projects.stream().map(project -> JdtUtils.getJavaProject(project))


Suggest using distinct() to avoid duplicates.

…into jinbo_sourcelookup

akaroml

Perfect. Thanks.

testforstephen added 6 commits October 25, 2017 10:50

Source lookup from project classpath entries

52ff6fd

Loop source containers to find the associated source file for the sta…

79886dd

…ck frame

Remove unused class

4ffd1b8

Add a workaround to fix jdt sourcelookup bug for java 9

22f8881

Merge branch 'master' of github.com:Microsoft/java-debug into jinbo_s…

5df54b6

…ourcelookup

Fix source lookup support for java 9

23006da

testforstephen changed the title ~~Looking up the stack frame's associated source file from source containers directly to improve search perf and bugs~~ Looking up the stack frame's associated source file from source containers directly to improve searching perf and bugs Dec 7, 2017

akaroml requested changes Dec 7, 2017

View reviewed changes

testforstephen added 2 commits December 8, 2017 10:46

fix review comments

939ff22

Refine javadoc

adb3895

akaroml requested changes Dec 8, 2017

View reviewed changes

yaohaizh reviewed Dec 8, 2017

View reviewed changes

Use stream api to optimize code

d086833

akaroml requested changes Dec 11, 2017

View reviewed changes

testforstephen added 3 commits December 11, 2017 15:32

Merge branch 'master' into jinbo_sourcelookup

d5fc90a

Use distinct api to remove duplicated projects

a2b7a25

Merge branch 'jinbo_sourcelookup' of github.com:Microsoft/java-debug …

6e81a6a

…into jinbo_sourcelookup

akaroml approved these changes Dec 11, 2017

View reviewed changes

testforstephen merged commit e5f1a0f into master Dec 11, 2017

testforstephen deleted the jinbo_sourcelookup branch December 11, 2017 09:34

testforstephen mentioned this pull request Dec 18, 2017

Improve the source lookup engine microsoft/vscode-java-debug#121

Closed

		@@ -164,17 +170,32 @@ public String[] getFullyQualifiedName(String uri, int[] lines, int[] columns) th

		@Override
		public String getSourceFileURI(String fullyQualifiedName, String sourcePath) {


		Set<IRuntimeClasspathEntry> calculated = new LinkedHashSet<>();

		projects.stream().map(project -> JdtUtils.getJavaProject(project))

Conversation

testforstephen commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

testforstephen Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

testforstephen Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaohaizh Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akaroml left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

testforstephen commented Dec 7, 2017 •

edited

Loading

testforstephen Dec 8, 2017 •

edited

Loading

testforstephen Dec 8, 2017 •

edited

Loading

yaohaizh Dec 8, 2017 •

edited

Loading