Streaming read support for the loose ODB backend by ethomson · Pull Request #4450 · libgit2/libgit2

ethomson · 2017-12-19T01:21:45Z

Change the streaming reader API a bit, to provide the type and length of the object when initializing the stream. This would be a breaking API change - except, of course, that nobody is actually using the streaming reader API since none of the backends support it. So I feel pretty good breaking this API that nobody could be using.

Add streaming reader support to the loose object ODB backend. I cleaned up a bit while I was in there to add some more tests for things like read_header which had no explicit tests (and in fact failed on some of the loose ODB test corpus), teach read_header how to cope with "packlike loose objects" (which were a weird loose object format that was attempted for a bit and will never actually be seen in the wild) and drop the odb_loose internal zstream abstraction layer that ultimately became git_zstream.

Note that this depends on #4443.

pks-t

Cool, it was quite an enjoyable read. Thanks for that ;)

Personally, I've got no problem with breaking the API. In theory, somebody could have his own ODB backend which already supports the streaming API, but I kind of doubt that.

pks-t · 2018-01-05T08:45:34Z

+	if (decompressed > head_len) {
+		stream->start_len = decompressed - head_len;
+		memcpy(stream->start, head + head_len, decompressed - head_len);
+	}


So we're initially decompressing up to 64 bytes as that is the maximum header length for any object in the stream. As we might be decompressing too much data, though, we have to retain the first leading bytes of the object itself (64 - header_len) in a buffer. Did I get that right?

pks-t · 2018-01-05T08:48:45Z

+	 */
+	if (stream->start_len && len) {
+		size_t chunk = MIN(stream->start_len, len);
+		memcpy(buffer, stream->start, chunk);


This logic doesn't look right. If len < stream->start_len, we have to copy multiple chunks from stream->start on successive calls. But you keep on copying the beginning of stream->start, and don't ever advance into the start buffer. So the caller will repeatedly see the first bytes of stream->start without seeing the rest of it.

I didn't see this being addressed. Do I miss something here?

Edit: nvm, getting fixed later. The fixup landed in d315374 (odb: loose object streaming for packlike loose objects, 2017-12-17), though. I guess this was not by intention.

pks-t · 2018-01-05T08:51:36Z

+		total += chunk;
+	}
+
+	return total;


I don't see any logic how we can handle and indicate an EOF to the caller. Is that being handled by the git_odb_stream?

We behave like read(2) by returning 0 at EOF.

pks-t · 2018-01-05T08:54:15Z

+	backend = (loose_backend *)_backend;
+	*stream_out = NULL;
+	*len_out = 0;
+	*type_out = 0;


Shouldn't that be GIT_OBJ_BAD instead? 0 indicates GIT_OBJ__EXT1.

Seems reasonable; fixed.

pks-t · 2018-01-05T08:55:29Z

+	hash_ctx = git__malloc(sizeof(git_hash_ctx));
+	GITERR_CHECK_ALLOC(hash_ctx);
+
+	if (locate_object(&object_path, backend, oid) < 0) {


Maybe put that check before the allocations?

pks-t · 2018-01-05T09:23:15Z

+	zstream->z.next_in = (Bytef *)zstream->in;
+	zstream->z.avail_in = (uInt)zstream->in_len;
+
+	if ((size_t)zstream->z.avail_in != zstream->in_len) {


Is this handling the case where z.avail_in = (uInt) zstream->in_len overflows? If so, a small comment might be worthwhile.

Updated to add a comment and an improved test.

pks-t · 2018-01-05T09:26:15Z

-
-	return 0;
-}
-


pks-t · 2018-01-05T09:26:44Z

+
+	if (out_len)
+		*out_len = used;
+


This change doesn't look related?

pks-t · 2018-01-05T09:32:27Z

+		error = read_header_loose_standard(out, obj, (size_t)obj_len);

-	if (!git_object_typeisloose(hdr.type)) {
+	if (!error && !git_object_typeisloose(out->type)) {


A goto in case of error would be a bit nicer here, I guess. Special-casing error handling just because it is the last statement only asks for small error later on when more code is being inserted after it.

pks-t · 2018-01-05T09:37:23Z

+	if (!is_zlib_compressed_data((unsigned char *)obj.ptr, obj.size))
 		error = read_loose_packlike(out, &obj);
 	else
 		error = read_loose_standard(out, &obj);


Huh. Why should we treat an object as packlike though when obj.size < 2? Shouldn't we error out in that case instead? Something like:

if (is_zlib_compressed_data(&is_compressed, obj.ptr. obj.size) < 0) return -1; if (!is_compressed) packlike(); else standard();

ethomson · 2018-01-29T21:52:38Z

I think that I've taken care of your issues, @pks-t

pks-t

I went through the complete series again. It's only stylistic things now I can comment on, otherwise it looks good to me.

pks-t · 2018-02-01T08:36:59Z

 	cl_assert_equal_oid(&expected, &oid);
 }

+void test_odb_largefiles__streamread(void)


Shouldn't 57ab26d (odb_loose: test reading a large file in stream, 2017-12-17
) come after 4fd05ca (odb: introduce streaming loose object reader, 2017-12-17)?

I don't think we've come to an agreement on this, no. I'll rearrange these, but we should hash this out at the summit. I'm definitely not the only person who prefers to add a failing test first, so the question is if we can get a workflow that accommodates all our preferences.

I definitly agree with you. Adding failing tests first is the best way how we can demonstrate that the next commit fixes the issue. See my pull request #4500, which tackles this exact problem

pks-t · 2018-02-01T08:40:11Z

 #include "git2/types.h"

+/* maximum possible header length */
+#define HEADER_LEN 64


Small nit: maybe call it MAX_HEADER_LEN?

pks-t · 2018-02-01T08:42:14Z

+	/* inflate the initial part of the compressed buffer in order to
+	 * parse the header; read the largest header possible, then push the
+	 * remainder into the body buffer.
+	 */


This comment doesn't follow kernel-style anymore. See Linus explaining it nicely: http://lkml.iu.edu/hypermail/linux/kernel/1607.1/00627.html

lololol, i trust that your ing with the "nicely".

Hehe, I am ;)

pks-t · 2018-02-01T08:44:55Z

+	loose_readstream *stream = (loose_readstream *)_stream;
+
+	git_futils_mmap_free(&stream->map);
+	git_zstream_free(&stream->zstream);


pks-t · 2018-02-01T08:45:20Z

+		total += chunk;
+	}
+
+	return total;


pks-t · 2018-02-01T08:45:48Z

+	 */
+	if (stream->start_len && len) {
+		size_t chunk = MIN(stream->start_len, len);
+		memcpy(buffer, stream->start, chunk);


I didn't see this being addressed. Do I miss something here?

Edit: nvm, getting fixed later. The fixup landed in d315374 (odb: loose object streaming for packlike loose objects, 2017-12-17), though. I guess this was not by intention.

pks-t · 2018-02-01T08:52:56Z

-		memcpy(buffer, stream->start, chunk);
+	if (start_remain && buffer_len) {
+		size_t chunk = min(start_remain, buffer_len);
+		memcpy(buffer, stream->start + stream->start_read, chunk);


Oops. Stumbled upon that the second time now :P

pks-t · 2018-02-01T08:56:54Z

+	* inflate the initial part of the compressed buffer in order to parse the
+	* header; read the largest header possible, then store it in the `start`
+	* field of the stream object.
+	*/


Small nit: indentation of this comment is a bit off here

pks-t · 2018-02-01T09:15:00Z


+void test_odb_loose__streaming_reads(void)
+{
+	size_t blocksizes[] = { 1, 2, 4, 16, 99, 1024, 123456789, 0 };


Ugh, I was really confused by the 0 blocksize here. Didn't see that it was used as a terminator, so I kept on asking myself for several minutes how we can actually fully read through the stream in that case. Maybe remove that blocksize and iterate by array size instead of by sentinel?

Yeah, I think you're right; I fixed this.

There are two streaming functions; one for reading, one for writing. Disambiguate function names between `stream` and `writestream` to make allowances for a read stream.

The streaming read functionality should provide the length and the type of the object, like the normal read functionality does.

Provide a streaming loose object reader.

Since some test situations may have generous disk space, but limited RAM (eg hosted build agents), test that we can stream a large file into a loose object, and then stream it out of the loose object storage.

A "packlike" loose object was a briefly lived loose object format where the type and size were encoded in uncompressed space at the beginning of the file, followed by the compressed object contents. Handle these in a streaming manner as well.

Refactor packlike loose object reads to use `git_zstream` for simplification.

Introduce `get_output_chunk` that will inflate/deflate all the available input buffer into the output buffer. `get_output` will call `get_output_chunk` in a loop, while other consumers can use it to inflate only a piece of the data.

Make `read_header` use the common zstream implementation. Remove the now unnecessary zlib wrapper in odb_loose.

Support `read_header` for "packlike loose objects", which were a temporarily and uncommonly used format loose object format that encodes the header before the zlib deflate data. This will never actually be seen in the wild, but add support for it for completeness and (more importantly) because our corpus of test data has objects in this format, so it's easier to support it than to try to special case it.

Test that we can read_header on large blobs. This should succeed on all platforms since we read only a few bytes into memory to be able to parse the header.

When checking to see if a file has zlib deflate content, make sure that we actually have read at least two bytes before examining the array.

Only run the large file tests on 64 bit platforms. Even though we support streaming reads on objects, and do not need to fit them in memory, we use `size_t` in various places to reflect the size of an object.

`MAX_HEADER_LEN` is a more descriptive constant name.

ethomson · 2018-02-02T00:54:08Z

Thanks again, updated with your comments.

pks-t · 2018-02-02T09:30:38Z

Now if only GitHub had an interface to easily view the interdiff between two versions of the PR... don't really feel like going through all of it again right now :(

ethomson force-pushed the ethomson/odb_loose_readstream branch 2 times, most recently from d2dcb41 to ad0ac75 Compare December 31, 2017 16:40

pks-t requested changes Jan 5, 2018

View reviewed changes

ethomson force-pushed the ethomson/odb_loose_readstream branch 2 times, most recently from d92533d to 72e661b Compare January 29, 2018 21:51

ethomson force-pushed the ethomson/odb_loose_readstream branch from 72e661b to ba5bb53 Compare January 29, 2018 22:53

pks-t reviewed Feb 1, 2018

View reviewed changes

ethomson added 15 commits February 1, 2018 16:35

odb_loose: stream -> writestream

c74e927

There are two streaming functions; one for reading, one for writing. Disambiguate function names between `stream` and `writestream` to make allowances for a read stream.

odb: provide length and type with streaming read

97f9a5f

The streaming read functionality should provide the length and the type of the object, like the normal read functionality does.

odb: introduce streaming loose object reader

b61846f

Provide a streaming loose object reader.

odb_loose: test reading a large file in stream

dbe3d3e

Since some test situations may have generous disk space, but limited RAM (eg hosted build agents), test that we can stream a large file into a loose object, and then stream it out of the loose object storage.

odb_loose: packlike loose objects use git_zstream

80dc394

Refactor packlike loose object reads to use `git_zstream` for simplification.

odb: test loose object streaming

b1e66bf

zstream: introduce a single chunk reader

6155e06

Introduce `get_output_chunk` that will inflate/deflate all the available input buffer into the output buffer. `get_output` will call `get_output_chunk` in a loop, while other consumers can use it to inflate only a piece of the data.

odb_loose: read_header should use zstream

4c7a16b

Make `read_header` use the common zstream implementation. Remove the now unnecessary zlib wrapper in odb_loose.

odb_loose: test read_header explicitly

e118231

odb_loose: test read_header on large blobs

27078e5

Test that we can read_header on large blobs. This should succeed on all platforms since we read only a few bytes into memory to be able to parse the header.

odb_loose: validate length when checking for zlib content

624614b

When checking to see if a file has zlib deflate content, make sure that we actually have read at least two bytes before examining the array.

odb_loose: largefile tests only on 64 bit platforms

909a199

Only run the large file tests on 64 bit platforms. Even though we support streaming reads on objects, and do not need to fit them in memory, we use `size_t` in various places to reflect the size of an object.

odb_loose: HEADER_LEN -> MAX_HEADER_LEN

09df354

`MAX_HEADER_LEN` is a more descriptive constant name.

ethomson force-pushed the ethomson/odb_loose_readstream branch from ba5bb53 to 09df354 Compare February 2, 2018 00:53

ethomson merged commit 0fd0bfe into master Feb 8, 2018

ethomson mentioned this pull request Apr 10, 2018

"error inflating zlib stream" when reading blobs > 4GB #4615

Closed

ethomson deleted the ethomson/odb_loose_readstream branch January 9, 2019 10:18

Uh oh!

Conversation

ethomson commented Dec 19, 2017

Uh oh!

pks-t 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!

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!

ethomson commented Jan 29, 2018

Uh oh!

pks-t 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!

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!

ethomson commented Feb 2, 2018

Uh oh!

pks-t commented Feb 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects