Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: provide package path for main packages to cmd/compile #34480

Open
mdempsky opened this issue Sep 23, 2019 · 4 comments

Comments

@mdempsky
Copy link
Member

commented Sep 23, 2019

Can cmd/go provide cmd/compile with the full package path to the source package, even when compiling main packages?

When benchmarking cmd/compile changes, it's useful to key stuff by myimportpath (i.e., the -p command-line flag) and just spit everything across an entire "go build -a std cmd" build into a single file, and then let benchcmp or benchstat handle it.

But this currently doesn't work for main packages, because cmd/go sets -p main for these:

pkgpath := p.ImportPath
if cfg.BuildBuildmode == "plugin" {
pkgpath = pluginPath(a)
} else if p.Name == "main" && !p.Internal.ForceLibrary {
pkgpath = "main"
}
gcargs := []string{"-p", pkgpath}

So you end up with a bunch of "BenchmarkFoo:main" lines, which muddle the benchcmp/benchstat output.

I figure two main options:

  1. Change cmd/go to just set -p to the package path regardless, and cmd/compile can rewrite it to "main" where/if necessary. (Looking briefly at myimportpath, some uses would be unaffected; but DWARF and the new ABI stuff might be impacted.)

  2. Add another command-line flag for cmd/go to provide the package path, which cmd/compile can use for tagging benchmarking data with instead.

/cc @rsc @aclements

@jayconrod

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

The compiler uses myimportpath for symbol names and such. But as you said we can rewrite it to "main" as necessary. So either option will work for this use case.

I'm slightly inclined to option 2, for an option specific for benchmarking. User could also overwrite it if necessary.

@jayconrod

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

For option 1, a binary may have multiple packages named main. That comes up a lot when testing a main package and when using -coverpkg with a pattern that covers multiple main packages. So don't rewrite in any context where this needs to be unique.

For option 2, would you want to use this for import paths (strings that appear in import declarations)? Import paths are user-friendly but may not be unique. Package paths are unique but less friendly (resolved vendor directories and such). Currently, when a package is compiled, the compiler does not know how it will be imported by other packages. A package may be imported with multiple distinct import paths because of minimal module compatibility, too, so we have to be careful here.

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2019

I'm slightly inclined to option 2, for an option specific for benchmarking. User could also overwrite it if necessary.

That makes sense.

So don't rewrite in any context where this needs to be unique.

Perhaps cmd/go could provide -p "foo/bar" -main instead of rewriting -p "foo/bar" into -p "main"? That would let cmd/compile know when it should use "main" instead.

For option 2, would you want to use this for import paths (strings that appear in import declarations)? Import paths are user-friendly but may not be unique. Package paths are unique but less friendly (resolved vendor directories and such).

Hm, so I want that when I run "go build [pkgs...]", that each individual compile operation has some unique stable identifier so I can correlate benchmarks across builds.

That makes me think we actually want the resolved package path, which then also means we can't always rely on -p even for non-main packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.