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

embed-go performance and memory usage #92

Closed
nkovacs opened this issue Feb 16, 2017 · 9 comments
Closed

embed-go performance and memory usage #92

nkovacs opened this issue Feb 16, 2017 · 9 comments

Comments

@nkovacs
Copy link
Contributor

nkovacs commented Feb 16, 2017

I've optimized embed-go a bit. The trick is that I don't write the file contents into the source code, just a placeholder, format the code with the placeholders, then use fasttemplate to replace the placeholders with the contents of the files, streaming it directly from the original files to the destination go file.

This means the files are never held in memory completely, so memory usage is much lower.
It also avoids running gofmt on a very large source code, which speeds up things a bit and also lowers memory usage.

Since gofmt doesn't always align struct values depending on their length, I had to add an empty line to make sure the code is always correctly formatted.

I also had to copy the code behind strconv.Quote, unfortunately the public API was not good enough, and performance was pretty bad with it.

With a fairly large box of 387 files, mostly javascript, the numbers are:
before:
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:01.18
Maximum resident set size (kbytes): 264668
after:
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.56
Maximum resident set size (kbytes): 9144

With a single, 80Mb file:
before:
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:06.59
Maximum resident set size (kbytes): 2213800
after:
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:03.03
Maximum resident set size (kbytes): 10192

I haven't tested it thoroughly and I still need to escape the fasttemplate placeholders in case they show up somewhere else in the generated code.

The code is in this branch: https://github.com/nkovacs/go.rice/tree/fasttemplate

What do you think?

@GeertJohan
Copy link
Owner

Wow, that is pretty neat! I'm just wondering: isn't there a package that does the encoding into a string? It feels a bit weird to have that as part of the embedding flow instead of a separate package.

@nkovacs
Copy link
Contributor Author

nkovacs commented Apr 12, 2017

I couldn't find one. Since strconv.Quote is good for 99% use cases, I guess no one needed to create one.

I've extracted it into a separate package and added some tests, including all the strconv.Quote tests from the standard library: https://github.com/nkovacs/streamquote

I'll update this PR tomorrow.

@GeertJohan
Copy link
Owner

That's awesome, thanks!

@nkovacs
Copy link
Contributor Author

nkovacs commented Apr 13, 2017

I optimized streamquote (got rid of a bunch of small slice allocations), helps a bit with large files (v1 is the previous version from february, v2 is with the optimization)

one big file:

metric master v1 v2
rice wall time 0:06.72 0:02.90 0:02.11
rice max resident (kbytes) 1988876 10244 4720
build wall time 0:04.17 0:04.31 0:04.38
build max resident (kbytes) 1023208 1028712 1040584

many small files:

metric master v1 v2
rice wall time 0:00.40 0:00.20 0:00.22
rice max resident (kbytes) 104248 9232 9180
build wall time 0:01.63 0:01.63 0:01.63
build max resident (kbytes) 181652 181008 190088

@GeertJohan
Copy link
Owner

GeertJohan commented Apr 18, 2017

Neat! Do you think this is ready for a PR, or do you still want to change some things?

@nkovacs
Copy link
Contributor Author

nkovacs commented Apr 18, 2017

I still need to fix filename escaping, if the filename contains {%%}, it might cause problems with fasttemplate.

@GeertJohan
Copy link
Owner

Filename escaping has been merged. Thanks!

Do you want to add more code related to this feature, or can we close this issue?

@nkovacs
Copy link
Contributor Author

nkovacs commented Apr 20, 2017

I just need to fix the issue in my previous comment, I'll have a PR up soon.

@GeertJohan
Copy link
Owner

Just merged the PR. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants