chunked: store cache as binary and use a bloom filter by giuseppe · Pull Request #1870 · containers/storage

giuseppe · 2024-03-27T11:47:13Z

The bloom filter itself is useful to reduce page faults with the mmap'ed cache files, as it reduces lookups.

Storing the file as a binary instead reduces the file size considerably, with the quay.io/giuseppe/zstd-chunked:fedora-{38,39,40}{,-updated} images I see:

before:

# find -name '=Y2h1bmtlZC1tYW5pZmVzdC1jYWNoZQ==' -exec stat -c '%s' \{\} \;2547644
2575163
2547644
2476816
2462835
2533346

after:

# find -name '=Y2h1bmtlZC1tYW5pZmVzdC1jYWNoZQ==' -exec stat -c '%s' \{\} \;
1319206
1312332
1275803
1270629
1297565

so it is ~50% size reduction

openshift-ci · 2024-03-27T11:47:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

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~~ [giuseppe]

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

giuseppe · 2024-03-27T11:50:29Z

@kolyshkin @mtrmac @rhatdan some more improvements to the cache file

mtrmac

Thanks so much for splitting this into smaller commits. That made the review very enjoyable.

pkg/chunked/cache_linux_test.go

mtrmac · 2024-03-27T19:42:34Z

pkg/chunked/cache_linux.go

 }

+func getBinaryDigest(stringDigest string) ([]byte, error) {
+	d, err := digest.Parse(stringDigest)


Consider having this function accept a digest.Digest instead, and pushing the digest.Parse to callers; on various code paths that compute the digest, that can avoid a digest.Validate (regex evaluation).

OTOH then the other code paths must call Parse or Validate if dealing with untrusted data.

pkg/chunked/cache_linux.go

mtrmac · 2024-03-27T19:45:23Z

pkg/chunked/cache_linux.go

+	if err != nil {
+		return nil, err
+	}
+	digest := append([]byte(d.Algorithm()+":"), digestBytes...)


(More space could be saved by using a single byte to index into a table of algorithm names, or something like that, at the cost of even more complexity. Possibly not worth it, and definitely not blocking this PR.)

mtrmac · 2024-03-27T21:16:02Z

pkg/chunked/cache_linux.go

+		bloomFilter: bloomFilter,
+		digestLen:   int(digestLen),
+		fnames:      fnames,
+		fnamesLen:   int(fnamesLen),


(Nit: I’d aesthetically prefer if the type declaration, and the two constructors, were all listing the fields in the same order — and if that order were somehow consistent. Here we have (data, len) for file names, but (len, data) for tags, for example.)

pkg/chunked/cache_linux.go

mtrmac · 2024-03-27T21:22:27Z

pkg/chunked/cache_linux_test.go

 				t.Error("wrong digest found")
 			}
-			expectedLocation := generateFileLocation(r.Name, uint64(r.ChunkOffset), uint64(r.ChunkSize))
+			expectedLocation, err := generateFileLocation(0, uint64(r.ChunkOffset), uint64(r.ChunkSize))


Non-blocking: Testing this with at least two distinct paths, to truly exercise the indexing logic, would be nice.

pkg/chunked/cache_linux.go

pkg/chunked/bloom_filter_test.go

giuseppe · 2024-04-08T11:13:28Z

the PR is ready for review

rhatdan · 2024-04-08T14:19:32Z

@mtrmac needs another review.

pkg/chunked/cache_linux.go

pkg/chunked/bloom_filter.go

pkg/chunked/cache_linux.go

mtrmac

I’m sorry about the delayed response.

pkg/chunked/cache_linux.go

giuseppe · 2024-04-09T08:41:03Z

I've fixed your comments, except #1870 (comment). What would you like me to do here?

mtrmac · 2024-04-09T15:14:16Z

pkg/chunked/cache_linux.go

 // are stored.  $DIGEST has length digestLen stored in the cache file file header.
-func generateTag(digest string, offset, len uint64) string {
-	return fmt.Sprintf("%s%.20d@%.20d", digest, offset, len)
+func appendTag(digest []byte, offset, len uint64) ([]byte, error) {


(Absolutely non-blocking: the error value is always nil now.)

pkg/chunked/bloom_filter.go

pkg/chunked/cache_linux.go

mtrmac · 2024-04-09T15:27:29Z

I've fixed your comments, except #1870 (comment). What would you like me to do here?

https://github.com/containers/storage/pull/1870/files#r1557856169 , or perhaps I’m missing something.

rhatdan · 2024-04-16T11:07:16Z

@giuseppe This is waiting on you now?

kolyshkin

Left some very minor comments. Overall it looks like a good case for protobuf, or is that an overkill?

It looks like appendBinaryDigest's first argument is always []byte{}. First, it could be nil, second, if it's not ever used maybe drop it (and rename the function to getBinaryDigest).

kolyshkin · 2024-04-16T18:58:56Z

pkg/chunked/cache_linux.go

 	var vdata bytes.Buffer
+	var tagsBuffer bytes.Buffer


nit:

var vdata, tagsBuffer bytes.Buffer

kolyshkin · 2024-04-16T19:29:08Z

pkg/chunked/cache_linux.go

 	nElements := len(cacheFile.tags) / cacheFile.tagLen

 	i := sort.Search(nElements, func(i int) bool {
-		d := byteSliceAsString(cacheFile.tags[i*cacheFile.tagLen : i*cacheFile.tagLen+cacheFile.digestLen])


nit: with this change, func byteSliceAsString (defined above) can also be removed.

kolyshkin · 2024-04-16T23:20:52Z

pkg/chunked/cache_linux.go

-func generateTag(digest string, offset, len uint64) string {
-	return fmt.Sprintf("%s%.20d@%.20d", digest, offset, len)
+func generateTag(digest []byte, offset, len uint64) []byte {
+	tag := append(digest[:], []byte(fmt.Sprintf("%.20d@%.20d", offset, len))...)


Can you please explain the need for [:] after digest here? I am staring at it and can't see why it's needed.

Also, []byte() conversion is redundant here, in Go you can append a string... to a []byte slice.

this code is replaced by a next commit. I'll squash the two patches so it will just disappear

kolyshkin · 2024-04-16T23:42:35Z

pkg/chunked/cache_linux.go

+	sort.Slice(tags, func(i, j int) bool {
+		return bytes.Compare(tags[i], tags[j]) == -1
+	})


Easier to use slices.SortFunc(tags, bytes.Compare) here

had to revert this change as we are using go 1.20

(BTW we are now agreed on updating to Go 1.21 (around containers/skopeo#2297 ). That would probably be a separate PR.)

kolyshkin · 2024-04-16T23:53:41Z

pkg/chunked/cache_linux.go

+	var tags [][]byte
 	for _, k := range toc {
 		if k.Digest != "" {
+			digest, err := appendBinaryDigest([]byte{}, k.Digest)


nit: you can use nil instead of []byte{} here -- appending to nil slice is fine.

kolyshkin · 2024-04-17T00:05:33Z

pkg/chunked/bloom_filter_test.go

 	var tagsBuffer bytes.Buffer
 	var vdata bytes.Buffer
+	var fnames bytes.Buffer


nit:

var tagsBuffer, vdata, fnames bytes.Buffer

(unless you don't like that style; to me it's less repetition)

kolyshkin · 2024-04-17T00:08:27Z

pkg/chunked/cache_linux.go

+		nameBytes := []byte(name)
+
+		if err := binary.Write(&fnames, binary.LittleEndian, uint32(len(nameBytes))); err != nil {
+			return 0, err
+		}
+		if _, err := fnames.Write(nameBytes); err != nil {
+			return 0, err
+		}


You can avoid []byte conversion here, using name directly (as in len(name) and fnames.WriteString(name))

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe · 2024-04-19T10:40:44Z

thanks @mtrmac and @kolyshkin. I've addressed your last comments and pushed a new version

mtrmac

LGTM otherwise.

pkg/chunked/cache_linux.go

use the binary representation for a given digest, it helps reducing the file size by ~25%. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

it helps reducing the cache file size by ~25%. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

use a bloom filter to speed up lookup of digests in a cache file. The biggest advantage is that it reduces page faults with the mmap'ed cache file. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

so that the same file path is stored only once in the cache file. After this change, the cache file measured on the fedora:{38,39,40} images is in average ~6% smaller. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

it reduces the cache file size by ~3%. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

mtrmac · 2024-04-19T19:34:59Z

/lgtm

Thanks!

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

openshift-ci bot added the approved label Mar 27, 2024

giuseppe changed the title ~~chunked: use a bloom filter to speed up cache lookup and store file as binary~~ chunked: store cache as binary and use a bloom filter Mar 27, 2024

giuseppe force-pushed the chunked-bloom-filter branch from f4f29dd to ba6657b Compare March 27, 2024 11:50

giuseppe force-pushed the chunked-bloom-filter branch 2 times, most recently from fb5a6e7 to de1dff9 Compare March 27, 2024 13:11

mtrmac reviewed Mar 27, 2024

View reviewed changes

mtrmac reviewed Mar 28, 2024

View reviewed changes

pkg/chunked/bloom_filter_test.go Outdated Show resolved Hide resolved

giuseppe force-pushed the chunked-bloom-filter branch from de1dff9 to 9f2b6a3 Compare March 28, 2024 22:22

giuseppe marked this pull request as ready for review April 4, 2024 07:41

openshift-ci bot removed the do-not-merge/work-in-progress label Apr 4, 2024

mtrmac reviewed Apr 8, 2024

View reviewed changes

pkg/chunked/cache_linux.go Outdated Show resolved Hide resolved

pkg/chunked/cache_linux.go Outdated Show resolved Hide resolved

pkg/chunked/bloom_filter.go Outdated Show resolved Hide resolved

pkg/chunked/cache_linux.go Outdated Show resolved Hide resolved

mtrmac reviewed Apr 8, 2024

View reviewed changes

pkg/chunked/cache_linux.go Outdated Show resolved Hide resolved

pkg/chunked/cache_linux.go Outdated Show resolved Hide resolved

giuseppe force-pushed the chunked-bloom-filter branch from 9f2b6a3 to d986219 Compare April 9, 2024 08:36

mtrmac reviewed Apr 9, 2024

View reviewed changes

pkg/chunked/cache_linux.go Outdated Show resolved Hide resolved

kolyshkin reviewed Apr 17, 2024

View reviewed changes

chunked: move cache file generation to separate function

397943b

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the chunked-bloom-filter branch from d986219 to 0820a2a Compare April 19, 2024 10:40

giuseppe force-pushed the chunked-bloom-filter branch from 0820a2a to 6d8fc8e Compare April 19, 2024 13:55

mtrmac reviewed Apr 19, 2024

View reviewed changes

pkg/chunked/cache_linux.go Outdated Show resolved Hide resolved

chunked: store digest in binary format

3347254

use the binary representation for a given digest, it helps reducing the file size by ~25%. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe added 6 commits April 19, 2024 21:28

chunked: store file offset and length in binary format

e6793e3

it helps reducing the cache file size by ~25%. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

chunked: add implementation for a bloom filter

6668761

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

chunked: use a bloom filter to speedup lookup

e9a96e0

use a bloom filter to speed up lookup of digests in a cache file. The biggest advantage is that it reduces page faults with the mmap'ed cache file. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

chunked: store file names separately

59ac039

so that the same file path is stored only once in the cache file. After this change, the cache file measured on the fedora:{38,39,40} images is in average ~6% smaller. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

chunked: store file locations as binary

9619a53

it reduces the cache file size by ~3%. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

chunked: bump version number for cache file

065a2f3

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the chunked-bloom-filter branch from 6d8fc8e to 065a2f3 Compare April 19, 2024 19:28

openshift-ci bot assigned mtrmac Apr 19, 2024

openshift-ci bot added the lgtm label Apr 19, 2024

openshift-merge-bot bot merged commit d227439 into containers:main Apr 19, 2024

Conversation

giuseppe commented Mar 27, 2024

Uh oh!

openshift-ci bot commented Mar 27, 2024

Uh oh!

giuseppe commented Mar 27, 2024

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

giuseppe commented Apr 8, 2024

Uh oh!

rhatdan commented Apr 8, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

giuseppe commented Apr 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mtrmac commented Apr 9, 2024

Uh oh!

rhatdan commented Apr 16, 2024

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giuseppe commented Apr 19, 2024

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mtrmac commented Apr 19, 2024