From: Junio C Hamano <hidden> Date: 2017-01-08 23:42:39
larsxschneider@gmail.com writes:
From: Lars Schneider <redacted>
Some `clean` / `smudge` filters might require a significant amount of
time to process a single blob. During this process the Git checkout
operation is blocked and Git needs to wait until the filter is done to
continue with the checkout.
Teach the filter process protocol (introduced in edcc858) to accept the
status "delayed" as response to a filter request. Upon this response Git
continues with the checkout operation and asks the filter to process the
blob again after all other blobs have been processed.
Hmm, I would have expected that the basic flow would become
for each paths to be processed:
convert-to-worktree to buf
if not delayed:
do the caller's thing to use buf
else:
remember path
for each delayed paths:
ensure filter process finished processing for path
fetch the thing to buf from the process
do the caller's thing to use buf
and that would make quite a lot of sense. However, what is actually
implemented is a bit disappointing from that point of view. While
its first part is the same as above, the latter part instead does:
for each delayed paths:
checkout the path
Presumably, checkout_entry() does the "ensure that the process is
done converting" (otherwise the result is simply buggy), but what
disappoints me is that this does not allow callers that call
"convert-to-working-tree", whose interface is obtain the bytestream
in-core in the working tree representation, given an object in the
object-db representation in an in-core buffer, to _use_ the result
of the conversion. The caller does not have a chance to even see
the result as it is written straight to the filesystem, once it
calls checkout_delayed_entries().
From: Jakub Narębski <hidden> Date: 2017-01-10 22:11:26
W dniu 09.01.2017 o 00:42, Junio C Hamano pisze:
larsxschneider@gmail.com writes:
quoted
From: Lars Schneider <redacted>
Some `clean` / `smudge` filters might require a significant amount of
time to process a single blob. During this process the Git checkout
operation is blocked and Git needs to wait until the filter is done to
continue with the checkout.
Lars, what is expected use case for this feature; that is when do you
think this problem may happen? Is it something that happened IRL?
quoted
Teach the filter process protocol (introduced in edcc858) to accept the
status "delayed" as response to a filter request. Upon this response Git
continues with the checkout operation and asks the filter to process the
blob again after all other blobs have been processed.
Hmm, I would have expected that the basic flow would become
for each paths to be processed:
convert-to-worktree to buf
if not delayed:
do the caller's thing to use buf
else:
remember path
for each delayed paths:
ensure filter process finished processing for path
fetch the thing to buf from the process
do the caller's thing to use buf
I would expect here to have a kind of event loop, namely
while there are delayed paths:
get path that is ready from filter
fetch the thing to buf (supporting "delayed")
if path done
do the caller's thing to use buf
(e.g. finish checkout path, eof convert, etc.)
We can either trust filter process to tell us when it finished sending
delayed paths, or keep list of paths that are being delayed in Git.
and that would make quite a lot of sense. However, what is actually
implemented is a bit disappointing from that point of view. While
its first part is the same as above, the latter part instead does:
for each delayed paths:
checkout the path
Presumably, checkout_entry() does the "ensure that the process is
done converting" (otherwise the result is simply buggy), but what
disappoints me is that this does not allow callers that call
"convert-to-working-tree", whose interface is obtain the bytestream
in-core in the working tree representation, given an object in the
object-db representation in an in-core buffer, to _use_ the result
of the conversion. The caller does not have a chance to even see
the result as it is written straight to the filesystem, once it
calls checkout_delayed_entries().
From: Taylor Blau <hidden> Date: 2017-01-10 23:33:22
On Tue, Jan 10, 2017 at 11:11:01PM +0100, Jakub Narębski wrote:
W dniu 09.01.2017 o 00:42, Junio C Hamano pisze:
quoted
larsxschneider@gmail.com writes:
quoted
From: Lars Schneider <redacted>
Some `clean` / `smudge` filters might require a significant amount of
time to process a single blob. During this process the Git checkout
operation is blocked and Git needs to wait until the filter is done to
continue with the checkout.
Lars, what is expected use case for this feature; that is when do you
think this problem may happen? Is it something that happened IRL?
quoted
quoted
Teach the filter process protocol (introduced in edcc858) to accept the
status "delayed" as response to a filter request. Upon this response Git
continues with the checkout operation and asks the filter to process the
blob again after all other blobs have been processed.
Hmm, I would have expected that the basic flow would become
for each paths to be processed:
convert-to-worktree to buf
if not delayed:
do the caller's thing to use buf
else:
remember path
for each delayed paths:
ensure filter process finished processing for path
fetch the thing to buf from the process
do the caller's thing to use buf
I would expect here to have a kind of event loop, namely
while there are delayed paths:
get path that is ready from filter
fetch the thing to buf (supporting "delayed")
if path done
do the caller's thing to use buf
(e.g. finish checkout path, eof convert, etc.)
We can either trust filter process to tell us when it finished sending
delayed paths, or keep list of paths that are being delayed in Git.
This makes a lot of sense to me. The "get path that is ready from filter" should
block until the filter has data that it is ready to send. This way Git isn't
wasting time in a busy-loop asking whether the filter has data ready to be sent.
It also means that if the filter has one large chunk that it's ready to write,
Git can work on that while the filter continues to process more data,
theoretically improving the performance of checkouts with many large delayed
objects.
quoted
and that would make quite a lot of sense. However, what is actually
implemented is a bit disappointing from that point of view. While
its first part is the same as above, the latter part instead does:
for each delayed paths:
checkout the path
Presumably, checkout_entry() does the "ensure that the process is
done converting" (otherwise the result is simply buggy), but what
disappoints me is that this does not allow callers that call
"convert-to-working-tree", whose interface is obtain the bytestream
in-core in the working tree representation, given an object in the
object-db representation in an in-core buffer, to _use_ the result
of the conversion. The caller does not have a chance to even see
the result as it is written straight to the filesystem, once it
calls checkout_delayed_entries().
In addition to the above, I'd also like to investigate adding a "no more items"
message into the filter protocol. This would be useful for filters that
batch delayed items into groups. In particular, if the batch size is `N`, and Git
sends `2N-1` items, the second batch will be under-filled. The filter on the
other end needs some mechanism to send the second batch, even though it hasn't
hit max capacity.
Specifically, this is how Git LFS implements object transfers for data it does
not have locally, but I'm sure that this sort of functionality would be useful
for other filter implementations as well.
--
Thanks,
Taylor Blau
ttaylorr@github.com
From: Lars Schneider <hidden> Date: 2017-01-11 10:20:54
On 10 Jan 2017, at 23:11, Jakub Narębski [off-list ref] wrote:
W dniu 09.01.2017 o 00:42, Junio C Hamano pisze:
quoted
larsxschneider@gmail.com writes:
quoted
From: Lars Schneider <redacted>
Some `clean` / `smudge` filters might require a significant amount of
time to process a single blob. During this process the Git checkout
operation is blocked and Git needs to wait until the filter is done to
continue with the checkout.
Lars, what is expected use case for this feature; that is when do you
think this problem may happen? Is it something that happened IRL?
Yes, this problem happens every day with filters that perform network
requests (e.g. GitLFS). In GitLFS we even implemented Git wrapper
commands to address the problem: https://github.com/git-lfs/git-lfs/pull/988
The ultimate goal of this patch is to be able to get rid of the wrapper
commands.
quoted
quoted
Teach the filter process protocol (introduced in edcc858) to accept the
status "delayed" as response to a filter request. Upon this response Git
continues with the checkout operation and asks the filter to process the
blob again after all other blobs have been processed.
Hmm, I would have expected that the basic flow would become
for each paths to be processed:
convert-to-worktree to buf
if not delayed:
do the caller's thing to use buf
else:
remember path
for each delayed paths:
ensure filter process finished processing for path
fetch the thing to buf from the process
do the caller's thing to use buf
I would expect here to have a kind of event loop, namely
while there are delayed paths:
get path that is ready from filter
fetch the thing to buf (supporting "delayed")
if path done
do the caller's thing to use buf
(e.g. finish checkout path, eof convert, etc.)
We can either trust filter process to tell us when it finished sending
delayed paths, or keep list of paths that are being delayed in Git.
From: Jakub Narębski <hidden> Date: 2017-01-11 14:53:37
W dniu 11.01.2017 o 11:20, Lars Schneider pisze:
On 10 Jan 2017, at 23:11, Jakub Narębski [off-list ref] wrote:
quoted
W dniu 09.01.2017 o 00:42, Junio C Hamano pisze:
quoted
larsxschneider@gmail.com writes:
quoted
From: Lars Schneider <redacted>
Some `clean` / `smudge` filters might require a significant amount of
time to process a single blob. During this process the Git checkout
operation is blocked and Git needs to wait until the filter is done to
continue with the checkout.
Lars, what is expected use case for this feature; that is when do you
think this problem may happen? Is it something that happened IRL?
Yes, this problem happens every day with filters that perform network
requests (e.g. GitLFS).
Do I understand it correctly that the expected performance improvement
thanks to this feature is possible only if there is some amount of
parallelism and concurrency in the filter? That is, filter can be sending
one blob to Git while processing other one, or filter can be fetching blobs
in parallel.
This means that filter process works as a kind of (de)multiplexer for
fetching and/or processing blob contents, I think.
[...] In GitLFS we even implemented Git wrapper
commands to address the problem: https://github.com/git-lfs/git-lfs/pull/988
The ultimate goal of this patch is to be able to get rid of the wrapper
commands.
I'm sorry, I don't see it how the wrapper helps here.
quoted
quoted
quoted
Teach the filter process protocol (introduced in edcc858) to accept the
status "delayed" as response to a filter request. Upon this response Git
continues with the checkout operation and asks the filter to process the
blob again after all other blobs have been processed.
Hmm, I would have expected that the basic flow would become
for each paths to be processed:
convert-to-worktree to buf
if not delayed:
do the caller's thing to use buf
else:
remember path
for each delayed paths:
ensure filter process finished processing for path
fetch the thing to buf from the process
do the caller's thing to use buf
I would expect here to have a kind of event loop, namely
while there are delayed paths:
get path that is ready from filter
fetch the thing to buf (supporting "delayed")
if path done
do the caller's thing to use buf
(e.g. finish checkout path, eof convert, etc.)
We can either trust filter process to tell us when it finished sending
delayed paths, or keep list of paths that are being delayed in Git.
You are talking about the "busy-loop" solution, isn't it? In the
same notation, it would look like this:
while there are delayed paths:
for each delayed path:
request path from filter [1]
fetch the thing (supporting "delayed") [2]
if path done
do the caller's thing to use buf
remove it from delayed paths list
Footnotes:
----------
1) We don't send the Git-side contents of blob again, isn't it?
So we need some protocol extension / new understanding anyway.
for example that we don't send contents if we request path again.
2) If path is not ready at all, filter protocol would send status=delayed
with empty contents. This means that we would immediately go to the
next path, if there is one.
There are some cases where busy loop is preferable, but I don't think
it is the case here.
The event loop solution would require additional protocol extension,
but I don't think those complicate protocol too much:
A. Git would need to signal filter process that it has sent all paths,
and that it should be sending delayed paths when they are ready. This
could be done for example with "command=continue".
packet: git> command=continue
B. Filter driver, in the event-loop phase, when (de)multiplexing fetching
or processing of data, it would need now to initialize transfer, instead
of waiting for Git to ask. It could look like this:
packet: git< status=resumed [3]
packet: git< pathname=file/to/be/resumed [4]
packet: git< 0000
packet: git< SMUDGED_CONTENT_CONTINUED
packet: git< 0000
packet: git< 0000 # empty list, means "status=success" [5]
Footnotes:
----------
3.) It could be "status=success", "status=delayed", "command=resumed", etc.
4.) In the future we can add byte at which we resume, size of file, etc.
5.) Of course sending reminder of contents may be further delayed.
--
Jakub Narębski
From: Lars Schneider <hidden> Date: 2017-01-11 09:44:27
On 09 Jan 2017, at 00:42, Junio C Hamano [off-list ref] wrote:
larsxschneider@gmail.com writes:
quoted
From: Lars Schneider <redacted>
Some `clean` / `smudge` filters might require a significant amount of
time to process a single blob. During this process the Git checkout
operation is blocked and Git needs to wait until the filter is done to
continue with the checkout.
Teach the filter process protocol (introduced in edcc858) to accept the
status "delayed" as response to a filter request. Upon this response Git
continues with the checkout operation and asks the filter to process the
blob again after all other blobs have been processed.
Hmm, I would have expected that the basic flow would become
for each paths to be processed:
convert-to-worktree to buf
if not delayed:
do the caller's thing to use buf
else:
remember path
for each delayed paths:
ensure filter process finished processing for path
fetch the thing to buf from the process
do the caller's thing to use buf
and that would make quite a lot of sense. However, what is actually
implemented is a bit disappointing from that point of view. While
its first part is the same as above, the latter part instead does:
for each delayed paths:
checkout the path
Presumably, checkout_entry() does the "ensure that the process is
done converting" (otherwise the result is simply buggy), but what
disappoints me is that this does not allow callers that call
"convert-to-working-tree", whose interface is obtain the bytestream
in-core in the working tree representation, given an object in the
object-db representation in an in-core buffer, to _use_ the result
of the conversion. The caller does not have a chance to even see
the result as it is written straight to the filesystem, once it
calls checkout_delayed_entries().
I am not sure I can follow you here. A caller of "convert_to_working_tree"
would indeed see filtered result. Consider the following example. The
filter delays the conversion twice and responds with the filtered results
on the third call:
CALL: int convert_to_working_tree(*src=='CONTENT', *dst, *delayed==0)
RESPONSE: return == 1; *delayed == 1, *dst==''
CALL: int convert_to_working_tree(*src=='CONTENT', *dst, *delayed==0)
RESPONSE: return == 1; *delayed == 1, *dst==''
CALL: int convert_to_working_tree(*src=='CONTENT', *dst, *delayed==0)
RESPONSE: return == 1; *delayed == 0, *dst=='FILTERED_CONTENT'
I implemented the "checkout_delayed_entries" function in v1 because
it solved the problem with minimal changes in the existing code. Our previous
discussion made me think that this is the preferred way:
I do not think we want to see such a rewrite all over the
codepaths. It might be OK to add such a "these entries are known
to be delayed" list in struct checkout so that the above becomes
more like this:
for (i = 0; i < active_nr; i++)
checkout_entry(active_cache[i], state, NULL);
+ checkout_entry_finish(state);
That is, addition of a single "some of the checkout_entry() calls
done so far might have been lazy, and I'll give them a chance to
clean up" might be palatable. Anything more than that on the
caller side is not.
c.f. http://public-inbox.org/git/xmqqvavotych.fsf@gitster.mtv.corp.google.com/
Thanks,
Lars