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

x/playground: feature request: prepopulate file system with contents of $GOROOT/src #23603

Open
jimmyfrasche opened this issue Jan 29, 2018 · 27 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@jimmyfrasche
Copy link
Member

Adding the $GOROOT contents to the playground file system would make it possible to use more of the go/* packages in the playground more easily.

This would be helpful for both reporting bugs in the go/* packages and for sharing demonstrations of how to use the go/* packages.

@bradfitz bradfitz changed the title playground feature request: prepopulate file system with contents of $GOROOT/src x/playground: feature request: prepopulate file system with contents of $GOROOT/src Jan 29, 2018
@gopherbot gopherbot added this to the Unreleased milestone Jan 29, 2018
@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 29, 2018
@bradfitz
Copy link
Contributor

Seems reasonable.

@mvdan
Copy link
Member

mvdan commented Jan 29, 2018

I'm fairly sure that fixing this would fix #19825 too.

@cznic
Copy link
Contributor

cznic commented Jan 29, 2018

How to resolve playground results caching vs different Go versions ie. different $GOROOT/src contents?

@andybons
Copy link
Member

Caching is keyed off of whatever version the playground is running: https://github.com/golang/playground/blob/master/sandbox.go#L83

So bugs reported for older (or non-stable) versions of Go would not be able to use this mechanism.

@ysmolski
Copy link
Member

ysmolski commented Mar 11, 2018

Adding the whole tree of /src into NaCl fake filesystem is expensive (100mb+ of data). Can anybody give a reasonable use case when accessing $GOROOT/src is required and will make a good example in docs, etc? I fail to see a strong reason for that.

@mvdan
Copy link
Member

mvdan commented Mar 11, 2018

Some tests and examples use files within the Go tree, such as go/parser using its own Go files in an example - #19823. Although it's not a terribly strong case; they could all be rewritten to embed small sample files.

@jimmyfrasche
Copy link
Member Author

@ysmolsky The motivating example was https://play.golang.org/p/r5oZfFZHIF8 from #23594 which made it harder to demonstrate the bug in that issue (well maybe not "harder" as much as not "extremely easy").

I've also written examples of how to use the go/* packages for people asking questions but had to explain that it didn't work on the playground and they had to go run it locally (which caused some confusion).

Adding a playable example for go/build.Import would be possible with small sample files in strings but it would also need to override a lot of the hooks in the build Context to access that faux-file, to the point where the example would mostly be doing that and only a small fraction of the example code would be about the example itself.

Anyway it's not like it has to download that 100mb into the browser or anything so it would only affect people running the playground.

@ysmolski
Copy link
Member

Fair enough. I will see what can be done and what it will cost.

@josharian
Copy link
Contributor

Aside: Are the nacl contents compressed? They should compress very well...

@ysmolski
Copy link
Member

ysmolski commented Mar 11, 2018

Building playground with fake fs including everything from $GOROOT/src, except /cmd and /syscall (biggest directories) resulted in the following binary sizes in the docker image:

11304359 Mar 11 22:32 /usr/local/go/bin/go
18278379 Mar 11 22:34 /usr/local/go/bin/nacl_amd64p32/go

Compared to the original sizes:

11304359 Feb 28 16:44 /usr/local/go/bin/go
8316907 Feb 28 16:46 /usr/local/go/bin/nacl_amd64p32/go

Avg response time from /compile endpoint for the default example ~ 430ms
Original time ~ 220ms

When add more content to fake fs of NaCL, we add overhead in the form of time needed to unzip the file pack src/syscall/fstest_nacl.go:

package syscall
import "sync"
func init() {
        var once sync.Once
        fsinit = func() {
                once.Do(func() {
                        unzip("\x50\x4b\x03\x04\.....

@josharian
Copy link
Contributor

Could we switch to lazily unzipping on a file-by-file basis?

@ysmolski
Copy link
Member

ysmolski commented Mar 11, 2018

More DYI benchmarks for src-included build. This program takes about 1100 ms to run in local playground:

package main

import (
	"fmt"
	"path/filepath"
)

func main() {
	m, err := filepath.Glob("/usr/local/go/src/archive/*")
	if err != nil { fmt.Println(err); return }
	fmt.Println(m)
}

Original playground runs it for ~ 245ms

@gopherbot
Copy link

Change https://golang.org/cl/100060 mentions this issue: go/parser: add example for using ParseDir with go/build

@ysmolski
Copy link
Member

@josharian the problem with file-by-file approach is that whenever syscall is used fake FS initialised at once in memory from big zip blog. If we were to init file on demand we would have to rebuild the whole thing from scratch.

@jimmyfrasche can we reduce to packages set from /src/ to some bare minimum required for example? do we really need all of them?

@jimmyfrasche
Copy link
Member Author

@ysmolsky it should be totally fine to ditch any testdata directory and an acceptable trade-off to dump cmd/ but I'm guessing the problem is deeper, based on your findings so far.

@josharian
Copy link
Contributor

@josharian the problem with file-by-file approach is that whenever syscall is used fake FS initialised at once in memory from big zip blog.

Yes, my suggestion was to not initialize the whole thing at once, but instead initialize only the directory layout and contents, and initialize individual files lazily as needed. But I haven't looked at all about what would be involved in doing that.

@josharian
Copy link
Contributor

I took a quick look at this. In syscall/unzip_nacl.go func unzip, all file contents are inflated and passed to func create (syscall/fs_nacl.go). ISTM you could do some specialization there and create a "zipped file" type that gets deflated only when read from.

@gopherbot
Copy link

Change https://golang.org/cl/100815 mentions this issue: go/doc: add examples for creating Package and Examples

@gopherbot
Copy link

Change https://golang.org/cl/101280 mentions this issue: go/importer: add example of importer.For

@gopherbot
Copy link

Change https://golang.org/cl/101281 mentions this issue: go/build: add example of build.Import

@gopherbot
Copy link

Change https://golang.org/cl/101286 mentions this issue: go/types: change examples to use source importer

@jimmyfrasche
Copy link
Member Author

@ysmolsky I've been looking through all of the stdlib examples and there are quite a few examples that access testdata but none of them would work even with this because they assume that the working directory == the package directory.

@jimmyfrasche
Copy link
Member Author

@ysmolsky have you had a chance to investigate @josharian's suggestion?

@ysmolski
Copy link
Member

ysmolski commented May 2, 2018

@jimmyfrasche I had a chance, but I do not see a way to implement this for now. :-(

@jimmyfrasche
Copy link
Member Author

So poking at the code, if I understand things, there needs to be two changes:

Change unzip to not decompress all the files when the file system is initialized but generate a func() ([]byte, error) (or equivalent) that decompresses the contents on demand for compressed files.

A createLazy in fs_nacl.go that works the same as create for directories, 0-length files and uncompressed files, but for files length > 0 that are compressed it takes the func() ([]byte, error) from the previous step. When the file is read from or written to it for the first time it calls that to populate the actual file contents; after that it behaves like a normal file.

Neither of those seem exactly easy to do, but it seems like they would be sufficient. Is that everyone else's read of the situation?

@josharian
Copy link
Contributor

Sounds plausible. Might also need to think about any concurrency requirements (need sync.Once?).

@jimmyfrasche
Copy link
Member Author

It would need something, definitely. Probably that just because it's the simplest, but it looks like everything in the nacl fs has lots of locking already and everywhere this would be needed would require locks so it might just need a wasDecompressed flag or somesuch.

@andybons andybons removed their assignment Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

8 participants