whyrusleeping / gx

A package management tool
MIT License
1.88k stars 111 forks source link

Unify dependency fetching #197

Closed schomatis closed 5 years ago

schomatis commented 5 years ago
gxutil: unify dependency fetching in `InstallDeps`

The previous implementation spawned multiple goroutines to fetch packages but
for the dependencies of one package at a time. This meant that the last
goroutine that took longer to finish (probably because it was fetching the
biggest dependency) held the start of more goroutines to fetch the dependencies
of the packages already fetched.

Unify all the dependencies in a single queue (abstracted in the new
`DependencyQueue` structure) so as soon as new dependencies of a fetched package
are available new goroutines can be spawned to fetch them.

Raise the number of parallel goroutines fetching packages (now `maxGoroutines`)
from 2 to 20 now that this bigger number can be leveraged from this unification.

Remove the retry logic around `GetPackageTo` (as that function already contains
code with equivalent functionality) to simplify the fetching part of the
function.

Split `InstallDeps`

Split the `InstallDeps` function in two functions which represent clear separate
processes: fetching packages (`fetchDependencies`) and running the `post-
install` hook on them (`dependenciesPostInstall`).

The `fetchDependencies` function is the previous `installDeps` function (now
renamed) without the `maybeRunPostInstall` call, which was encapsulated in its
own `dependenciesPostInstall` function that replicates the same DFS traversal of
the dependency DAG to run the hook.

This commit permits a more clear understanding of the process of installing
dependencies which clears the road for the unification of the fetching functions
into one.

There is a count discrepancy bug in the `ProgMeter` introduced in this commit
but it will be fixed in the next commit when `fetchDependencies` is refactored
to eliminate the recursive implementation.

Closes #195.

schomatis commented 5 years ago

Unrelated test error fixed in #198, needs to rebase after merge.

schomatis commented 5 years ago

I have to finish the documentation and fix some minor details but this change already shows a considerable performance improvement. Before finishing this up I need a green light from @whyrusleeping to raise the ratelim (now called maxGoroutines) from the original value of 2 (now 20, but could be any number) as I'm not sure what was the original rationale for this low number, but raising it was the original motivation behind this entire PR.

schomatis commented 5 years ago

@Stebalien WDYT about raising the number of goroutines fetching dependencies?

Stebalien commented 5 years ago

WDYT about raising the number of goroutines fetching dependencies?

If it helps, do it.

schomatis commented 5 years ago

@Stebalien Ready for review.

schomatis commented 5 years ago

@Stebalien Fixed. Thanks for catching the goroutines bug!

whyrusleeping commented 5 years ago

Oh, this is nice! Thanks @schomatis!

I hacked a towards making this a tiny bit cleaner in #206, but this is far cleaner. If @Stebalien is happy with the changes, i'd be down to merge this, then rebase #206 on top and make the 'cache fetch linking' stuff there use this new code.

Stebalien commented 5 years ago

Yes, this looks good now.