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

single packages.Load for NameForPackage #944

Closed

Conversation

vikstrous
Copy link
Contributor

@vikstrous vikstrous commented Nov 26, 2019

This is one of the optimizations suggested in #918. The original suggestion was to load every package that needs to be loaded in a single packages.Load call. I ran into issues trying to implement that because packages.Load has flags that define how much data should be loaded. It turned out that if we load all data for all relevant packages, that actually slows things down. Instead, I batched the loads of packages for calls to NameForPackage.

I tested this change on top of all of #942 #941 #940.

This is what the timings looked like before this commit (in my private codebase):

load autobind 1.893891096s
binder load time 2.383718776s
build time 4.523106863s
load time 548.545523ms
load time 503.184846ms
load time 513.424915ms
load time 628.098503ms
load time 570.70579ms
load time 508.176378ms
load time 480.067102ms
load time 495.141265ms
load time 544.685734ms
load time 537.060982ms
load time 510.831309ms
load time 544.138726ms
load time 580.567556ms
load time 601.751553ms
load time 583.932285ms
load time 893.051704ms
load time 835.748479ms
load time 854.607076ms
load time 1.057673489s
load time 602.564898ms
load time 654.87885ms
load time 652.425622ms
load time 652.79054ms
generate time 16.909831482s
29.26user 24.03system 0:23.90elapsed 222%CPU (0avgtext+0avgdata 198584maxresident)k
0inputs+0outputs (1major+459356minor)pagefaults 0swaps

After this commit:

load autobind 2.05095349s
binder load time 2.620924994s
load time 719.630163ms
build time 5.659686914s
generate time 2.255979872s
12.99user 8.68system 0:10.07elapsed 215%CPU (0avgtext+0avgdata 196372maxresident)k
0inputs+0outputs (1major+182189minor)pagefaults 0swaps

build and generate time correspond to the two main parts of api/generate.go.
load time is the time spent in packages.Load for the purposes of NameForPackage. As you can see, the combined packages.Load call is moved from the Load phase into the Build phase but even though so many more packages are being loaded, it takes about as long as a single one of the later calls.

I didn't see an improvement when this commit is applied without the others, but I didn't test it extensively by itself.

I need feeback on how NameForPackage is passed through the various structs and also on the name of the new struct.

Results when directly applied to master by itself:
before:

load autobind 1.957855281s
binder load time 18.537183239s
load autobind 2.677523501s
binder load time 15.554334922s
build time 18.423786225s
load time 926.142032ms
load time 1.027569047s
load time 618.982484ms
load time 581.960888ms
load time 638.869157ms
load time 563.321504ms
load time 555.43315ms
load time 536.209612ms
load time 589.453371ms
load time 563.956469ms
load time 553.15628ms
load time 566.709479ms
load time 558.047512ms
load time 660.112754ms
load time 608.56697ms
load time 794.513994ms
load time 712.262528ms
load time 775.887765ms
load time 710.474496ms
load time 726.115501ms
load time 652.594165ms
load time 668.182971ms
load time 736.236443ms
generate time 26.084938958s
87.87user 75.67system 1:10.62elapsed 231%CPU (0avgtext+0avgdata 196956maxresident)k
0inputs+0outputs (1major+785188minor)pagefaults 0swaps

after:

load autobind 1.970106545s
binder load time 13.081893745s
load autobind 2.489918625s
binder load time 18.986631712s
load time 13.878410384s
build time 35.582962805s
generate time 11.627699718s
82.33user 69.53system 1:07.58elapsed 224%CPU (0avgtext+0avgdata 186284maxresident)k
0inputs+0outputs (1major+635377minor)pagefaults 0swaps

I can't explain those results. There must be something weird with calling packages.Load multiple times on the same packages.

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 16:40
@vikstrous vikstrous mentioned this pull request Nov 26, 2019
@vektah vektah closed this Jan 15, 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.

2 participants