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

deduplicate package load between AutoBind and Binder #945

Closed
wants to merge 2 commits into from

Conversation

vikstrous
Copy link
Contributor

@vikstrous vikstrous commented Nov 26, 2019

This is one of the optimizations suggested in #918.

Improvements against master:

Before:

auto bind load 2.684139918s
binder load 19.267647024s
auto bind load 3.140190561s
binder load 19.725400875s
build time 23.085849454s

After:

combined load 17.965141019s
combined load 17.479315829s
build time 17.721352532s

"build time" is the time spent in the build phase of api/generate.go. The first set of loads is part of the models plugin and the second is part of the core functionality.

Note that this reorders InjectBuiltins and AutoBind. I'm not sure what the consequences of that are.

I'm surprised that tests didn't break without 8c110b7 but generation in our private repo did. I don't fully understand this code yet. I'd appreciate tips for adding a test for it.

I have:

  • Added tests covering the bug / feature (see testing)
  • n/a Updated any relevant documentation (see docs)

@vikstrous vikstrous mentioned this pull request Nov 26, 2019
2 tasks
@vikstrous vikstrous marked this pull request as ready for review November 26, 2019 18:57
@vikstrous vikstrous mentioned this pull request Nov 26, 2019
@vikstrous vikstrous force-pushed the no-double-load branch 3 times, most recently from 0f2413b to 15e59e8 Compare January 9, 2020 05:39
@vikstrous
Copy link
Contributor Author

Rebased.
New unscientific benchmark compared to master:

before:

46.94user 27.35system 0:31.48elapsed 236%CPU (0avgtext+0avgdata 193760maxresident)k
0inputs+0outputs (1major+401610minor)pagefaults 0swaps

after:

42.29user 24.44system 0:29.39elapsed 227%CPU (0avgtext+0avgdata 199044maxresident)k
0inputs+0outputs (1major+382901minor)pagefaults 0swaps

I also tested on top of #983 and saw the same 3 second improvement. That seems to be how long the package loading is taking for me.

@coveralls
Copy link

coveralls commented Jan 19, 2020

Coverage Status

Coverage decreased (-0.04%) to 63.415% when pulling fbb20b9 on vikstrous:no-double-load into ec4f6b1 on 99designs:master.

@vikstrous
Copy link
Contributor Author

New comparison vs master:

This PR:
23.82user 9.75system 0:16.15elapsed 207%CPU (0avgtext+0avgdata 189272maxresident)k
0inputs+0outputs (3major+229930minor)pagefaults 0swaps
23.14user 8.50system 0:14.69elapsed 215%CPU (0avgtext+0avgdata 191284maxresident)k
0inputs+0outputs (1major+200711minor)pagefaults 0swaps

master:
32.79user 13.08system 0:20.07elapsed 228%CPU (0avgtext+0avgdata 191624maxresident)k
0inputs+0outputs (1major+289754minor)pagefaults 0swaps
33.82user 13.26system 0:20.65elapsed 227%CPU (0avgtext+0avgdata 194580maxresident)k
0inputs+0outputs (1major+257803minor)pagefaults 0swaps

@vvakame vvakame requested a review from vektah January 21, 2020 03:25
@vektah
Copy link
Collaborator

vektah commented Feb 4, 2020

Most of the wiring in this PR has been superseded by #988.

Getting the same performance gains should be relatively straight forward if we can track down why ReferencedPackages isn't covering all of your deps. Happy to jump on a call (gophers?) if you want a hand to figure this out.

@vektah vektah closed this Feb 4, 2020
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.

3 participants