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

Speedups for precommit hooks #11841

Merged
merged 7 commits into from
Aug 11, 2015
Merged

Speedups for precommit hooks #11841

merged 7 commits into from
Aug 11, 2015

Conversation

eparis
Copy link
Contributor

@eparis eparis commented Jul 24, 2015

This (along with the separate munger change) greatly reduces the pre-commit hook time. From like 30+ seconds down to 8.5 seconds to build everything, 2.5 seconds to get swagger (that stinks) and another 2 seconds to do everything else in the hook.

Notice this does make it easier for the gendeepcopy and genconversion to be wrong when run by hand. This is the exact same as gen* can be wrong when run by hand if you didn't build new copies. The git pre-commit hook builds everything, so it should always be right (as would travis et al once you open a PR)

@k8s-bot
Copy link

k8s-bot commented Jul 24, 2015

GCE e2e build/test failed for commit 1bc3b2e587cb94c538f8952812f4eb398ab0d52e.

@mikedanese
Copy link
Member

@thockin

@zmerlynn zmerlynn closed this Jul 25, 2015
@zmerlynn zmerlynn reopened this Jul 25, 2015
@zmerlynn
Copy link
Member

So this now even more assumes that your local build is up to date, it looks like? i.e. if you haven't done a make in a little while, your precommits might just fail? (which sucks for some workflows, like git bisect)

find "${tmpdir}" -exec rsync -pt {} "${dest}" \; >/dev/null
files=($(find "${tmpdir}" -type f))
if [[ "${#files[@]}" -ne 0 ]]; then
cp -f "${files[@]}" "${dest}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this does the right thing. Consider:

thockin@freakshow:/tmp$ rm -rf a z

thockin@freakshow:/tmp$ mkdir -p a/b/c/d

thockin@freakshow:/tmp$ touch a/b/c/d/file-in-a-b-c-d

thockin@freakshow:/tmp$ touch a/b/c/file-in-a-b-c

thockin@freakshow:/tmp$ mkdir -p z/b/c/d

thockin@freakshow:/tmp$ find a -type f
a/b/c/d/file-in-a-b-c-d
a/b/c/file-in-a-b-c

thockin@freakshow:/tmp$ find z -type f

thockin@freakshow:/tmp$ f=($(find a -type f))

thockin@freakshow:/tmp$ cp -f "${f[@]}" z

thockin@freakshow:/tmp$ find z -type f
z/file-in-a-b-c
z/file-in-a-b-c-d

@thockin
Copy link
Member

thockin commented Aug 7, 2015

Eric,

Are you still working on this. It's driving me batty a little more each day...

@eparis
Copy link
Contributor Author

eparis commented Aug 7, 2015

i keep hoping to get back to it every afternoon. today looks ok.

Using the hack/ version is probably a little slower, but it still only
takes about .2 seconds. So probably worth the reduction in code.
@eparis eparis force-pushed the commit-speed branch 2 times, most recently from 6bce02d to 1212aab Compare August 10, 2015 23:09
@k8s-bot
Copy link

k8s-bot commented Aug 10, 2015

GCE e2e build/test passed for commit 1212aab78bdf50e4492fdd932e9e2e8c43a659ac.

@eparis
Copy link
Contributor Author

eparis commented Aug 11, 2015

hopefully other comments have been addressed. PTAL.

@k8s-bot
Copy link

k8s-bot commented Aug 11, 2015

GCE e2e build/test passed for commit f97e17680e8a6fd3ef709e0f9d7bfb631ba9ec7e.

@k8s-bot
Copy link

k8s-bot commented Aug 11, 2015

GCE e2e build/test failed for commit 81ab88cbedab2c1519c528323bb70985c242a255.

@thockin
Copy link
Member

thockin commented Aug 11, 2015

LGTM

@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2015
@eparis
Copy link
Contributor Author

eparis commented Aug 11, 2015

@k8s-bot test this

@k8s-bot
Copy link

k8s-bot commented Aug 11, 2015

GCE e2e build/test passed for commit 81ab88cbedab2c1519c528323bb70985c242a255.

@cjcullen
Copy link
Member

@eparis Do you want to squash commits before I merge?

eparis added 3 commits August 11, 2015 14:20
We were setting etcd values, but we started etcd before we set them.
Thankfully we were just setting them to etcd defaults, so it all worked
It isn't used anywhere, why have it?
Hooks seems like they should be hooks. Not sure hack makes a lot more
sense, but it has more stuff already.
eparis added 2 commits August 11, 2015 14:20
Just to get everything in one place...
Right now some of the hack/* tools use `go run` and build almost every
time. There are some which expect you to have already run `go install`.
And in all cases the pre-commit hook, which runs a full build wouldn't
want to do either, since it just built!

This creates a new hack/after-build/ directory and has the scripts which
REQUIRE that the binary already be built. It doesn't test and complain.
It just fails miserably. Users should not be in this directory. Users
should just use hack/verify-* which will just do the build and then call
the "after-build" version. The pre-commit hook or anything which KNOWS
the binaries have been built can use the fast version.
Instead of calling rsync over and over and over and over and over and
over and over and over and over (and probably over) use one `cp`

Before:
real	0m5.247s
user	0m2.294s
sys	0m1.300s

After:
real	0m2.260s
user	0m2.230s
sys	0m0.936s
@eparis
Copy link
Contributor Author

eparis commented Aug 11, 2015

squashed as seemed appropriate... each commit should be its own thing now.

@k8s-bot
Copy link

k8s-bot commented Aug 11, 2015

GCE e2e build/test passed for commit efd71090799138feb3d949ea50e37f89e3fc6b4c.

@k8s-bot
Copy link

k8s-bot commented Aug 11, 2015

GCE e2e build/test passed for commit bf9c3cd.

@cjcullen
Copy link
Member

Thanks!

cjcullen added a commit that referenced this pull request Aug 11, 2015
@cjcullen cjcullen merged commit 9fdd793 into kubernetes:master Aug 11, 2015
@eparis eparis deleted the commit-speed branch August 11, 2015 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants