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

schema/gen/go: batch file writes via a bytes.Buffer #161

Merged
merged 1 commit into from
Apr 9, 2021
Merged

schema/gen/go: batch file writes via a bytes.Buffer #161

merged 1 commit into from
Apr 9, 2021

Conversation

mvdan
Copy link
Contributor

@mvdan mvdan commented Apr 9, 2021

(see commit message)

@mvdan mvdan requested a review from warpfork April 9, 2021 13:15
@mvdan
Copy link
Contributor Author

mvdan commented Apr 9, 2021

There are better libraries that perform better at atomic file replaces, like google/renameio, but I feel like that's a bit overkill for a code generator right now - especially given the added dependency.

@mvdan
Copy link
Contributor Author

mvdan commented Apr 9, 2021

Uh, weird that the tests broke. Looking.

With this change, running 'go generate ./...' on the entire module while
running gopls on one of its files drops gopls's CPU spinning from ~25s
to well under a second. They should improve that anyway, but there's no
reason for the tens of thousands of tiny FS writes on our end either.

The time to run 'go generate ./...' itself is largely unaffected; it
goes from ~1.2s to ~1.1s, judging by a handful of runs.
@mvdan
Copy link
Contributor Author

mvdan commented Apr 9, 2021

Ah, the tests run in parallel, so no buffer reuse. Should be green now.

@warpfork
Copy link
Collaborator

warpfork commented Apr 9, 2021

Okay, phew, that was the one thing I was going to comment made me uncomfortable anyway 😅 (admittedly, unexported, so maybe could've been fine, but, still.)

@warpfork warpfork merged commit 7bca6b4 into ipld:master Apr 9, 2021
@mvdan mvdan deleted the generate-buffer branch May 24, 2021 08:51
@aschmahmann aschmahmann mentioned this pull request Aug 23, 2021
62 tasks
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

Successfully merging this pull request may close these issues.

2 participants