Use -workDir on gopkgs to improve performance of packages listing #1658

uudashr · 2018-05-05T06:29:17Z

Use -workDir this should improve the performance.

The difference on my case:
Without -workDir

time gopkgs |wc -l
    5822
gopkgs  1.95s user 2.57s system 121% cpu 3.740 total
wc -l  0.00s user 0.00s system 0% cpu 3.739 total

vs
Using -workDir

time gopkgs -workDir=.|wc -l
     836
gopkgs -workDir=.  0.28s user 0.31s system 225% cpu 0.261 total
wc -l  0.00s user 0.00s system 1% cpu 0.259 total

The duration 3.740 vs 0.261, the different around 3 sec.

uudashr · 2018-05-05T09:44:36Z

Test on server failed.

I'cant test on my local machine, using node version 10. I always got TypeError: Cannot read property 'packageJSON' of undefined.
Not sure it caused by node or by the vscode. @ramya-rao-a maybe you know anything about this?

uudashr · 2018-05-05T10:29:14Z

Nevermind.. fixed. Just need to delete the yarn.lock and node_modules

uudashr · 2018-05-05T18:17:46Z

@ramya-rao-a done.. review please

ramya-rao-a · 2018-05-09T20:52:54Z

src/goPackages.ts

-		const cmd = cp.spawn(getBinPath('gopkgs'), ['-format', '{{.Name}};{{.ImportPath}}'], { env: getToolsEnvVars() });
+		const args = ['-format', '{{.Name}};{{.ImportPath}}'];
+		if (workDir) {
+			args.push('-workDir', workDir);


This will fail for users who don't have the latest gopkgs i.e they have a version where workDir is not a supported.
You'll have to something similar to https://github.com/Microsoft/vscode-go/blob/0.6.79/src/goFormat.ts#L33 to prompt the user to update their tool, and fallback to not using the workDir

ramya-rao-a · 2018-05-09T20:53:10Z

src/goPackages.ts


+			gopkgs().then((pkgMap) => {


pass workDir here

ramya-rao-a · 2018-05-09T20:53:17Z

src/goPackages.ts

-		allPkgsLastHit = new Date().getTime();
-		return Promise.resolve(allPkgsCache);
+		cache.lastHit = new Date().getTime();
+		return Promise.resolve(cache.entry);
 	}

 	return getAllPackagesNoCache().then((pkgs) => {


pass workDir here

…ir flag

…into gopkgs-workdir

uudashr · 2018-05-10T16:48:13Z

Wow... I missed a lot. Fixed.

ramya-rao-a · 2018-05-10T21:59:24Z

src/goImport.ts

@@ -13,9 +13,9 @@ import { getImportablePackages } from './goPackages';

 const missingToolMsg = 'Missing tool: ';

-export function listPackages(excludeImportedPkgs: boolean = false): Thenable<string[]> {
+export function listPackages(excludeImportedPkgs: boolean = false, workDir?: string): Thenable<string[]> {


why is this change needed when getImportablePackages can calculate the workDir by using the given fileName?

I need this to pass the test of "Replace vendor packages with relative path" it will fail on the assert of "Relative path for vendor package 9fans.net/go/acme not found".

This happen because gopkgs recognize vscode workspace as workDir, and the workspace dir for the running test is vscode-go/out/test, no vendor package under the directory. While actually we are testing vendor package on under $GOPATH/src/github.com/rogpeppe/godef, there the vendor package of 9fans.net/go/acme should be found.

So it only for satisfying the test. The actual running vscode will never use workDir param or listPackages() (which kind of silly). I can't guess the project root/workDir based on fileName. Any suggestion?

Available the options are:

Guess golang project dir by filename

Change the test workspace dir to $GOPATH/src/github.com/rogpeppe/godef above test

Pass workDir to listPackages() (currently use this)

@uudashr This can happen IRL too. A user might open and edit a Go file that is not under the current workspace.

ramya-rao-a · 2018-05-10T22:10:42Z

src/goPackages.ts


+			gopkgs(workDir).then((pkgMap) => {
+				gopkgsRunning.delete(workDir);


Entries never get deleted from gopkgsRunning when gopkgs returns a rejected promise

No rejection on gopkgs. Positive and negative result will be return by resolve().

Base on your comment, better if negative result should be return by `reject(). Such as:

When gopkgs binary not found

When gopkgs needs to update

This is part of quality I think, which I'm okay, we can do this. But maybe after this PR.
Someday this need to be work on, if this not fixed soon, then any other contributor will follow the existing code style (in term of quality)

ramya-rao-a · 2018-05-10T22:38:09Z

src/goPackages.ts

@@ -29,6 +40,11 @@ function gopkgs(): Promise<Map<string, string>> {
 				return promptForMissingTool('gopkgs');
 			}

+			if (errchunks && errchunks.join('').startsWith('flag provided but not defined: -workDir')) {
+				promptForUpdatingTool('gopkgs');
+				return Promise.resolve(pkgs);


fallback to not using workdir by return gopkgs().then(result => resolve(result)) instead.

Ahh... good idea

ramya-rao-a · 2018-05-11T18:48:19Z

src/goPackages.ts

-				let subs = gopkgsSubscriptions;
-				gopkgsSubscriptions = [];
+		// Ensure only single gokpgs running
+		if (!gopkgsRunning.has(workDir)) {


@uudashr Say user edits 2 files in 2 different packages and for some reason gopkgs is slow. Then we will have 2 instances of the gopkgs running. Once the user is in the second file, is it really important to finish the gopkgs call for the first file?

Since it already run, we can use the cache. User can go back to the first file, and we already have the cache or running gopkgs.

Made some changes... all the go files under the same package root will share the same cache and gopkgs running instance.

@uudashr I was just thinking about the same thing over the weekend :) The fact that vendor packages under a higher ancestor is also available to the current file

Ahaha.. cool 👍

uudashr · 2018-05-11T20:16:54Z

Fail the test. Mac machine on Travis CI seems out of resources

uudashr · 2018-05-12T00:31:57Z

All good now

ramya-rao-a · 2018-05-14T07:18:14Z

@uudashr When I tried to test, gopkgs fails with cannot Stat: stat <mygopath>/src/github.com/prometheus/procfs/fixtures/26231/usr/bin/vim: no such file or directory

uudashr · 2018-05-14T09:36:57Z

@ramya-rao-a which test? on travis all good.
Is there a way to reproduce?

uudashr · 2018-05-14T10:04:04Z

Issues on gopkgs... for symbolic link

uudashr · 2018-05-14T11:50:42Z

Issues on the dependencies godirwalk. Just send the owner a PR karrick/godirwalk#4.

Meanwhile the gopkgs use the patched version and already work as expected. Please re-install the tools.

@ramya-rao-a could you re-run the test on your machine?

ramya-rao-a · 2018-05-15T01:11:41Z

Re-installing gopkgs fails with "unexpected directory layout" for the godirwalk. Had to delete the gopkgs folder and then install from scratch

And now I see cannot Stat: stat <mygopath>/src/github.com/prometheus/procfs/fixtures/26231/ns/mnt:[...] no such file or directory

uudashr · 2018-05-15T01:44:07Z

Due to this issue golang/go#21057
You have to execute another go get -u -github.com/uudashr/gopkgs/cmd/gopkgs if the first attempt fail.

Try to add -v go get -u -v -github.com/uudashr/gopkgs/cmd/gopkgs so we know what happen.

Let me know if still fail, I have to find another way then.

ramya-rao-a · 2018-05-15T01:53:23Z

Second update worked. I'll run some tests tomorrow and get back here.

uudashr · 2018-05-15T02:20:19Z

Awesome.. thanks

ramya-rao-a · 2018-05-16T02:32:45Z

src/goPackages.ts

@@ -149,12 +182,21 @@ export function getImportablePackages(filePath: string, useCache: boolean = fals
 				return;
 			}

-			let relativePkgPath = getRelativePackagePath(currentFileDirPath, currentWorkspace, pkgPath);
+			if (!foundPkgRootDir) {


@uudashr
You will have to find the pkgRootDir upfront and not after getting the results from gopkgs. To understand why, take the below case

If you dont have the godef code, then run go get github.com/rogpeppe/godef

Open the godef folder and then open the file ast.go in it

Run the command Go: Add Import with the current Go extension and then with your changes and search for acme

In the current Go extension, you will get the result for "9fans.net/go/acme", but you wont get the same with your changes.

This is because, the acme package is in the vendor folder which is not a sibling of the directory to which the file ast.go file belongs.

No, it should works.

Don't know which version gopkgs did you use. Since I push the latest changes, gopkgs -workDir also works on sub packages. Try to update the gopkgs

Run this command

$ gopkgs -format '{{.Name}};{{.ImportPath}}' -workDir=$(go env GOPATH)/src/github.com/rogpeppe/godef/go/ast|grep acme acme;github.com/rogpeppe/godef/vendor/9fans.net/go/acme

And this also covered on the test case "Replace vendor packages with relative path"

Works in my Mac but not Windows
I have the latest gopkgs in both
The "Replace vendor packages with relative path" test fails in Windows as well

Just push an update... would you mind to test it on Windows?

uudashr · 2018-05-16T13:21:17Z

I don't know why the lint is always failed on travis. All good on my machine.

And there is nothing wwrong with line 128

src/goMain.ts[128, 6]: Missing semicolon

ramya-rao-a · 2018-05-16T20:43:14Z

@uudashr That's because linting was failing on master which I just fixed. Sorry about that

uudashr · 2018-05-16T21:02:46Z

@ramya-rao-a no problem. If you have time please run the test on Windows.

ramya-rao-a · 2018-05-16T21:48:38Z

@uudashr The problem in Windows needed your fix as well as 0055c20.

I made one more commit to print the error message from gopkgs to the console. This should help in troubleshooting as I ran into gopkgs failure myself a few times as discussed in this PR.

I also added a retry logic for when the "unexpected directory layout" error is encountered.

The unit tests are passing on my Windows machine.

ramya-rao-a · 2018-05-16T22:16:40Z

I've updated #1490 with the means to try out these changes

Use -workdir on gopkgs

ab1e29d

Fallback to no-workdir cache

c546c88

Put subscription list to the map

ce666b0

Merge branch 'master' into gopkgs-workdir

5cc0c68

ramya-rao-a suggested changes May 9, 2018

View reviewed changes

uudashr added 5 commits May 10, 2018 22:06

Pass workDir to getAllPackagesNoCache

c0e7346

Pass workDir to gopkgs

8644ef0

Prompt update for new gopkgs tool when gopkgs does not support -workD…

b1c9e2b

…ir flag

Merge branch 'gopkgs-workdir' of https://github.com/uudashr/vscode-go …

0ccd844

…into gopkgs-workdir

Fix some test related to packages lookup

5ef2c3d

ramya-rao-a reviewed May 10, 2018

View reviewed changes

uudashr added 3 commits May 11, 2018 09:45

Fallback to gopkgs() when gpkgs version doesn't support -workDir

b7d4f20

Use opened file dir as workDir

f09a29f

preload packages no longer required

3085069

ramya-rao-a reviewed May 11, 2018

View reviewed changes

Cleanup

986301e

uudashr closed this May 12, 2018

uudashr reopened this May 12, 2018

Use shared cache for all files on the same package root

7220b10

uudashr closed this May 12, 2018

uudashr reopened this May 12, 2018

Merge branch 'master' into gopkgs-workdir

07d2793

ramya-rao-a reviewed May 16, 2018

View reviewed changes

Fix package root path on win os

50a885f

uudashr closed this May 16, 2018

uudashr reopened this May 16, 2018

uudashr closed this May 16, 2018

uudashr reopened this May 16, 2018

ramya-rao-a added 4 commits May 16, 2018 14:04

Fix for windows

0055c20

Better fix for Windows

92f2a0b

Print err msg from gopkgs to console

892846b

Retry when unexpected directory layout error is found

1cf42fe

ramya-rao-a approved these changes May 16, 2018

View reviewed changes

Show retrying message before retrying

6b87dce

ramya-rao-a merged commit c726cd1 into microsoft:master May 16, 2018


		gopkgs(workDir).then((pkgMap) => {
		gopkgsRunning.delete(workDir);

Use -workDir on gopkgs to improve performance of packages listing #1658

Use -workDir on gopkgs to improve performance of packages listing #1658

Conversation

uudashr commented May 5, 2018 • edited Loading

uudashr commented May 5, 2018

uudashr commented May 5, 2018

uudashr commented May 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uudashr commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uudashr May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uudashr May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uudashr commented May 11, 2018 • edited Loading

uudashr commented May 12, 2018

ramya-rao-a commented May 14, 2018

uudashr commented May 14, 2018

uudashr commented May 14, 2018

uudashr commented May 14, 2018 • edited Loading

ramya-rao-a commented May 15, 2018 • edited Loading

uudashr commented May 15, 2018

ramya-rao-a commented May 15, 2018

uudashr commented May 15, 2018

Choose a reason for hiding this comment

uudashr May 16, 2018 • edited Loading

Choose a reason for hiding this comment

ramya-rao-a May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uudashr commented May 16, 2018

ramya-rao-a commented May 16, 2018

uudashr commented May 16, 2018

ramya-rao-a commented May 16, 2018 • edited Loading

ramya-rao-a commented May 16, 2018

uudashr commented May 5, 2018 •

edited

Loading

uudashr commented May 5, 2018 •

edited

Loading

uudashr May 11, 2018 •

edited

Loading

uudashr May 11, 2018 •

edited

Loading

uudashr commented May 11, 2018 •

edited

Loading

uudashr commented May 14, 2018 •

edited

Loading

ramya-rao-a commented May 15, 2018 •

edited

Loading

uudashr May 16, 2018 •

edited

Loading

ramya-rao-a May 16, 2018 •

edited

Loading

ramya-rao-a commented May 16, 2018 •

edited

Loading