git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Lars Schneider <larsxschneider@gmail.com>
Cc: Eric Wong <e@80x24.org>, git@vger.kernel.org
Subject: Re: RFC: Enable delayed responses to Git clean/smudge filter requests
Date: Tue, 15 Nov 2016 10:03:10 -0800	[thread overview]
Message-ID: <xmqqvavotych.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <5BC69AC1-5499-4F73-816A-D8786106D796@gmail.com> (Lars Schneider's message of "Tue, 15 Nov 2016 15:29:09 +0100")

Lars Schneider <larsxschneider@gmail.com> writes:

>> The filter itself would need to be aware of parallelism
>> if it lives for multiple objects, right?
>
> Correct. This way Git doesn't need to deal with threading...

I think you need to be careful about three things (at least; there
may be more):

 * Codepaths that check out multiple cache entries do rely on the
   order of checkout.  We checkout removals first to make room so
   that creation of a path X can succeed if an existing path X/Y
   that used to want to see X as a directory can succeed (see the
   use of checkout_entry() by "git checkout", which does have two
   separate loops to explicitly guarantee this), for example.  I
   think "remove all and then create" you do not specifically have
   to worry about with the proposed change, but you may need to
   inspect and verify there aren't other kind of order dependency.

 * Done naively, it will lead to unmaintainable code, like this:

   + struct list_of_cache_entries *list = ...;
     for (i = 0; i < active_nr; i++)
   -    checkout_entry(active_cache[i], state, NULL);
   +    if (checkout_entry(active_cache[i], state, NULL) == DELAYED)
   +       add_cache_to_queue(&list, active_cache[i]);
   + while (list) {
   +    wait_for_checkout_to_finish(*list);
   +    list = list->next;
   + }

   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.

 * You'd need to rein in the maximum parallelism somehow, as you do
   not want to see hundreds of competing filter processes starting
   only to tell the main loop over an index with hundreds of entries
   that they are delayed checkouts.


  reply	other threads:[~2016-11-15 18:03 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-11-14 21:09 RFC: Enable delayed responses to Git clean/smudge filter requests Lars Schneider
2016-11-15  1:03 ` Eric Wong
2016-11-15 14:29   ` Lars Schneider
2016-11-15 18:03     ` Junio C Hamano [this message]
2016-11-16  9:53       ` Lars Schneider
2016-11-16 18:15         ` Junio C Hamano
2016-11-16 18:47           ` Lars Schneider
2016-11-16 19:19             ` Junio C Hamano
2016-11-16 22:41         ` Jakub Narębski
2016-11-16 23:46           ` Junio C Hamano
2016-11-17  9:19             ` Lars Schneider
2016-11-24 15:45       ` Lars Schneider
2016-11-28 21:48         ` Junio C Hamano
2016-11-15 18:27     ` Eric Wong
2017-01-09 20:44 ` Stefan Beller
2017-01-11 12:57   ` Lars Schneider

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqvavotych.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=e@80x24.org \
    --cc=git@vger.kernel.org \
    --cc=larsxschneider@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).