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

Reduce sorts done by ListImagesWithOptions #2338

Merged
merged 2 commits into from Aug 15, 2019
Merged

Reduce sorts done by ListImagesWithOptions #2338

merged 2 commits into from Aug 15, 2019

Conversation

@squaremo
Copy link
Member

squaremo commented Aug 8, 2019

The PR as a whole is intended to address #2328 by reducing the number of sorts, since that was identified as a bottleneck there.

The API ListImagesWithOptions as called by Weave Cloud asks for pretty much all the fields, so the first commit here will not make a big difference -- there will still be one sort per container. It may avoid some allocation, and some calculation in other uses (i.e., when not called by Weave Cloud).

The second commit caches the result of sorting by timestamp, when that is done. It will be effective to the extent that

  • the same image is used in more than one container
  • containers are using tag filters that ordered the images by timestamp (i.e., everything but semver)

There are some modest improvements that could be made with more invasive changes. Possibly the biggest improvement would be to sort images (actually the image tags) when they are fetched, rather than when they are used, since the latter will usually outnumber the former by orders of magnitude.

@squaremo squaremo force-pushed the do-less-sorting branch from 6c98cd0 to cd37ae6 Aug 8, 2019
@squaremo squaremo changed the title Only calculate container fields that are requested Reduce sorts done by ListImagesWithOptions Aug 8, 2019
@squaremo squaremo force-pushed the do-less-sorting branch from 7d0729e to d229dd3 Aug 8, 2019
@squaremo squaremo marked this pull request as ready for review Aug 12, 2019
@squaremo squaremo force-pushed the do-less-sorting branch from d229dd3 to 1fa9c25 Aug 12, 2019
@stefanprodan stefanprodan added this to the 1.14.0 milestone Aug 13, 2019
@squaremo squaremo force-pushed the do-less-sorting branch from 1fa9c25 to d39a3a5 Aug 15, 2019
squaremo added 2 commits Aug 7, 2019
The ListImagesWithOptions API method lets you supply a list of the
fields you want for each container, so that you can cut down on the
size of the response when you don't care about some fields. But all
the fields are calculated, whatever you ask for, which involves a lot
of sorting and filtering.

This commit makes the procedure calculating the fields _only_
calculate the fields that were actually requested. Some of the
calculations depend on the same intermediate results; so, the approach
is to lazily calculate and cache intermediate results.
Much of the time, the images returned from the ListImagesWithOptions
API are sorted according to timestamp. As a cheap and simple
optimisation for this case, when we're constructing a result, cache
both the slice of image.Info records and those records sorted by
timestamp.

This means, for any given request, images that occur in more than one
container will be sorted by timestamp only once.
@squaremo squaremo force-pushed the do-less-sorting branch from d39a3a5 to 1578a12 Aug 15, 2019
@squaremo squaremo merged commit 2f37429 into master Aug 15, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: helm Your tests passed on CircleCI!
Details
@squaremo squaremo deleted the do-less-sorting branch Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.