libimage: ListImages: pre-compute dangling/parent by vrothberg · Pull Request #1372 · containers/common

vrothberg · 2023-03-20T09:54:14Z

Checking whether an image is dangling and finding a parent image requires building a layer tree. Computing a layer tree is expensive, so add options to ListImages to pre-compute the dangling and parent information ahead of time; that requires 1 layer tree instead of N. The information will then be cached in the images such that subsequent calls to IsDangling() and Parent() can return the cached data.

Context: containers/podman/issues/17828

openshift-ci · 2023-03-20T09:54:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhatdan · 2023-03-20T10:24:29Z

LGTM

vrothberg · 2023-03-20T13:14:52Z

With this change listing 1400 images changes from ..

real    0m4.632s
user    0m10.341s
sys     0m0.591s

... to ...

real    0m0.868s
user    0m1.038s
sys     0m0.288s

The difference grows exponentially with the number of images.

mtrmac

I completely support the idea of ListImages computing all of this in one go (eventually, in the future, it could be a single SQLite read transaction), and ListImages callers opting into computing the expensive parts.

I’m not at all sure about storing the data in Image objects like this. AFAIK the parent/child relationships can change by adding/removing a different image (i.e. calling reload on the image that is caching data wouldn’t help).

All the pre-existing cache fields are values that don’t change over the lifetime of the image. And looking at methods like Image.Tag and Image.Mount, it seems quite possible to me that some caller could hold an Image object for a long time.

I’d rather prefer adding Runtime.ListImagesExtra which returns []ImageWithExtra, a new struct with extra fields.

mtrmac · 2023-03-20T16:27:14Z

libimage/runtime.go

+	// as the layer tree will computed once for all instead of once for
+	// each individual image (see containers/podman/issues/17828).
+
+	tree, err := r.layerTree()


Note that filterImages can also trigger a layerTree computation.

I suppose a clean way to deal with that could be to have a separate lazyLayerTree holder, passed to the various callees of of ListImages (and to things like getChildren?). That could also avoid the need for the if !(options.Dangling || options.Parent) above.

Asymptotically, calling layerTree once vs. twice makes no difference, and it’s a much smaller performance win than the primary purpose of this PR, so I’m absolutely fine with not dealing with the two calls in this PR. Maybe just a comment to note this duplication?

vrothberg · 2023-03-20T16:48:20Z

I’d rather prefer adding Runtime.ListImagesExtra which returns []ImageWithExtra, a new struct with extra fields.

Could we instead add a Dangling *bool and Parent *Image field to the Image struct and document that the data may be outdated? This way, IsDangling() and Patent() would always generate fresh data.

I am personally not that worried about potentially outdated data because, in theory, many attributes are outdated on return. But the current caching would prevent users from getting fresh data if needed, which is not good.

mtrmac · 2023-03-20T16:54:44Z

I’d rather prefer adding Runtime.ListImagesExtra which returns []ImageWithExtra, a new struct with extra fields.

Could we instead add a Dangling *bool and Parent *Image field to the Image struct and document that the data may be outdated?

If that is intended to still cache data, that would be inconvenient to use by callers (OTOH that would make it more convenient to ask for fresh data, which is, probably, good).

Or just avoid the separate WithExtra types, but still use the same semantics — those values are missing (nil?) unless a caller explicitly asks ListImages to populate them; and other ways to obtain an Image never populate them.

I am personally not that worried about potentially outdated data because, in theory, many attributes are outdated on return.

That’s a very fair point, all of the data is conceptually outdated by the time the store is unlocked — the image might not exist at all when ListImages returns.

vrothberg · 2023-03-24T10:18:28Z

@mtrmac PTanotherL. I think we're converging but want your blessing before adding tests and wrapping up.

mtrmac

Overall, given the very true point that all of the data is necessarily outdated by the time the caller receives it, I don’t feel that strongly about the design any more — both this, and the caching, is fair enough.

I have some weak opinions, but I’m fine with the current approach in this PR.

libimage/image.go

Checking whether an image is dangling and finding a parent image requires building a layer tree. Computing a layer tree is expensive, so add options to `ListImages` to pre-compute the dangling and parent information ahead of time; that requires 1 layer tree instead of N. Context: containers/podman/issues/17828 Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

mtrmac

LGTM, with or without the naming nit.

libimage/runtime.go

Computing the layer tree requires listing all images. Certain code paths have all images at hand already, so let's optimize a bit to avoid listing them redundantly. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

mtrmac

/lgtm

libimage: ListImages: pre-compute dangling/parent

mtrmac · 2023-03-28T20:37:30Z

@vrothberg there are merge conflicts

This reverts commit 349b951. PR containers/pull/1372 hasn't been fully applied by the openshift-ci bot,so let's start with a clean state again. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

Checking whether an image is dangling and finding a parent image requires building a layer tree. Computing a layer tree is expensive, so add options to `ListImages` to pre-compute the dangling and parent information ahead of time; that requires 1 layer tree instead of N. Further add an images argument to computing the layer tree. Context: containers/podman/issues/17828 Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

vrothberg · 2023-03-29T07:33:51Z

@vrothberg there are merge conflicts

Should be resolved now. That was quiet ugly. It seems like the openshift-ci bot has applied some but not all commits in the PR. That in combination with me applying some changes to the wrong commit made it ugly.

So I reverted the already applied commit and squashed the two old ones together.

vrothberg · 2023-03-29T11:20:33Z

@Luap99 PTAL, feel free to merge

Luap99 · 2023-03-29T11:31:16Z

/lgtm

Luap99 · 2023-03-29T11:31:59Z

Should be resolved now. That was quiet ugly. It seems like the openshift-ci bot has applied some but not all commits in the PR. That in combination with me applying some changes to the wrong commit made it ugly.

Sounds like a bug with the bot.

openshift-ci bot added the do-not-merge/work-in-progress label Mar 20, 2023

openshift-ci bot added the approved label Mar 20, 2023

vrothberg mentioned this pull request Mar 20, 2023

Running "podman images" and "podman images -a" is very slow containers/podman#17828

Closed

mtrmac reviewed Mar 20, 2023

View reviewed changes

vrothberg force-pushed the image-list-cache branch from a127be7 to 0a3efbb Compare March 24, 2023 10:18

mtrmac reviewed Mar 24, 2023

View reviewed changes

libimage/image.go Outdated Show resolved Hide resolved

vrothberg force-pushed the image-list-cache branch from 0a3efbb to 5e751d5 Compare March 27, 2023 11:37

vrothberg marked this pull request as ready for review March 27, 2023 11:57

openshift-ci bot removed the do-not-merge/work-in-progress label Mar 27, 2023

mtrmac reviewed Mar 27, 2023

View reviewed changes

libimage/runtime.go Outdated Show resolved Hide resolved

layerTree: add images argument

349b951

Computing the layer tree requires listing all images. Certain code paths have all images at hand already, so let's optimize a bit to avoid listing them redundantly. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

vrothberg force-pushed the image-list-cache branch from 5e751d5 to 349b951 Compare March 28, 2023 09:05

mtrmac reviewed Mar 28, 2023

View reviewed changes

openshift-ci bot assigned mtrmac Mar 28, 2023

openshift-ci bot added the lgtm label Mar 28, 2023

Merge pull request containers#1372 from vrothberg/image-list-cache

2008316

libimage: ListImages: pre-compute dangling/parent

vrothberg added 2 commits March 29, 2023 09:29

Revert "layerTree: add images argument"

e0956fe

This reverts commit 349b951. PR containers/pull/1372 hasn't been fully applied by the openshift-ci bot,so let's start with a clean state again. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

openshift-ci bot removed the lgtm label Mar 29, 2023

openshift-ci bot assigned Luap99 Mar 29, 2023

openshift-ci bot added the lgtm label Mar 29, 2023

openshift-merge-robot merged commit e27c30e into containers:main Mar 29, 2023

vrothberg deleted the image-list-cache branch March 29, 2023 11:55

Conversation

vrothberg commented Mar 20, 2023

Uh oh!

openshift-ci bot commented Mar 20, 2023

Uh oh!

rhatdan commented Mar 20, 2023

Uh oh!

vrothberg commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

vrothberg commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac commented Mar 20, 2023

Uh oh!

vrothberg commented Mar 24, 2023

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac commented Mar 28, 2023

Uh oh!

vrothberg commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vrothberg commented Mar 29, 2023

Uh oh!

Luap99 commented Mar 29, 2023

Uh oh!

Luap99 commented Mar 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vrothberg commented Mar 20, 2023 •

edited

Loading

vrothberg commented Mar 20, 2023 •

edited

Loading

vrothberg commented Mar 29, 2023 •

edited

Loading