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

Comments

Projects
None yet
8 participants
@jimmyfrasche
Copy link
Member

commented Jan 29, 2018

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

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

Seems reasonable.

@mvdan

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

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

@cznic

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2018

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

@andybons

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

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.

@ysmolsky

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2018

@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.

@ysmolsky

This comment has been minimized.

Copy link
Member

commented Mar 11, 2018

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

@josharian

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2018

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

@ysmolsky

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2018

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

@ysmolsky

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

commented Mar 12, 2018

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

@ysmolsky

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

@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

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2018

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

This comment has been minimized.

Copy link

commented Mar 14, 2018

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

@gopherbot

This comment has been minimized.

Copy link

commented Mar 18, 2018

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

@gopherbot

This comment has been minimized.

Copy link

commented Mar 18, 2018

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

@gopherbot

This comment has been minimized.

Copy link

commented Mar 18, 2018

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

@jimmyfrasche

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2018

@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

This comment has been minimized.

Copy link
Member Author

commented May 1, 2018

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

@ysmolsky

This comment has been minimized.

Copy link
Member

commented May 2, 2018

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

@jimmyfrasche

This comment has been minimized.

Copy link
Member Author

commented May 2, 2018

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

This comment has been minimized.

Copy link
Contributor

commented May 2, 2018

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

@jimmyfrasche

This comment has been minimized.

Copy link
Member Author

commented May 2, 2018

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.

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