Re: [PATCH v7 02/12] preload-index: add override to enable testing preload-index

[Stefan Beller]

> To execute the preload code path, we need a minimum of 2 cache entries and 2
> threads so that each thread actually has work to do.  Otherwise the logic
> below that divides the work up would need to be updated as well.  The
> additional complexity didn't seem worth it just to enable the code path to
> execute with a single thread on a single cache entry.

[snip]

>
> That would require a larger patch that would update the work division and
> thread creation logic for little to no gain.

Oh I was not aware of the additional needed refactoring. (I assumed that
would *just work*, experience should have told me to have a look first).

>
> There are no new tests that take advantage of this new environment variable.
> Instead you can run the entire git test suite with it by running:
>
> GIT_FORCE_PRELOAD_TEXT=1 prove -j12 --state=all ./t[0-9]*.sh
>
> I can add that to the commit message to make it more obvious if desired.

it would have helped me being less confused as there was an "obvious"
easier and more correct alternative, which you just explained
is neither easier nor more correct as we probably do not desire that
anyway.

Thanks,
Stefan

Re: [PATCH v7 02/12] preload-index: add override to enable testing preload-index

[Ben Peart]

On 9/20/2017 6:06 PM, Stefan Beller wrote:

On Tue, Sep 19, 2017 at 12:27 PM, Ben Peart  wrote:

Preload index doesn't run unless it has a minimum number of 1000 files.
To enable running tests with fewer files, add an environment variable
(GIT_FORCE_PRELOAD_TEST) which will override that minimum and set it to 2.


'it' being the number of threads ('it' was not mentioned before,
so reading the commit message confused me initially)



Signed-off-by: Ben Peart 
---
  preload-index.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/preload-index.c b/preload-index.c
index 70a4c80878..75564c497a 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -79,6 +79,8 @@ static void preload_index(struct index_state *index,
 return;

 threads = index->cache_nr / THREAD_COST;
+   if ((index->cache_nr > 1) && (threads < 2) && 
getenv("GIT_FORCE_PRELOAD_TEST"))
+   threads = 2;


Adding these lines is just a bandaid to trick


 if (threads < 2)
 return;


to not return early as the commit message does not discuss why
we set it to 2.



To execute the preload code path, we need a minimum of 2 cache entries 
and 2 threads so that each thread actually has work to do.  Otherwise 
the logic below that divides the work up would need to be updated as 
well.  The additional complexity didn't seem worth it just to enable the 
code path to execute with a single thread on a single cache entry.



Do we need threads at all for these tests, or would a patch like

-if (threads < 2)
+if (threads < 2 && !GIT_FORCE_PRELOAD_TEST)
  return;

work as well?


That would require a larger patch that would update the work division 
and thread creation logic for little to no gain.




That way tests can use any number of threads, though
they currently have no way of overriding the heuristic, yet.

With this alternative patch, it sounds to me as if the
issues kept more orthogonal. (specifically "Do we use preload?"
and "How many threads?". One could imagine that we later
want to introduce GIT_PRELOAD_THREADS for $reasons
and that would go over well in combination with
GIT_FORCE_PRELOAD_TEST)



I'm not sure why someone would want to test varying numbers of threads 
as that isn't a case that can ever actually happen with the existing 
code/feature.


I was just enabling testing of the code path with fewer than 1000 files 
as when I made my changes, I discovered that 1) there were no test cases 
for the core.preloadindex feature at all and 2) there was no way to run 
the existing tests with core.preloadindex as they don't have the minimum 
number of files.  This patch allows you to run the existing test suite 
with preload_index turned on for any test case that has 2 or more files 
(it passes by the way :)).



It seems to be only an internal test variable, such that we do
not need documentation. (Is that worth mentioning in the
commit message?)


Correct.  That is the reason I used the magic GIT_***_TEST naming 
pattern as that is the only way to ensure the environment variable is 
preserved when running the tests.




The test to make use of this new variable is found
in another patch I presume?



There are no new tests that take advantage of this new environment 
variable.  Instead you can run the entire git test suite with it by running:


GIT_FORCE_PRELOAD_TEXT=1 prove -j12 --state=all ./t[0-9]*.sh

I can add that to the commit message to make it more obvious if desired.


Stefan

Re: [PATCH v7 02/12] preload-index: add override to enable testing preload-index

[Stefan Beller]

On Tue, Sep 19, 2017 at 12:27 PM, Ben Peart  wrote:
> Preload index doesn't run unless it has a minimum number of 1000 files.
> To enable running tests with fewer files, add an environment variable
> (GIT_FORCE_PRELOAD_TEST) which will override that minimum and set it to 2.

'it' being the number of threads ('it' was not mentioned before,
so reading the commit message confused me initially)

>
> Signed-off-by: Ben Peart 
> ---
>  preload-index.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/preload-index.c b/preload-index.c
> index 70a4c80878..75564c497a 100644
> --- a/preload-index.c
> +++ b/preload-index.c
> @@ -79,6 +79,8 @@ static void preload_index(struct index_state *index,
> return;
>
> threads = index->cache_nr / THREAD_COST;
> +   if ((index->cache_nr > 1) && (threads < 2) && 
> getenv("GIT_FORCE_PRELOAD_TEST"))
> +   threads = 2;

Adding these lines is just a bandaid to trick

> if (threads < 2)
> return;

to not return early as the commit message does not discuss why
we set it to 2.

Do we need threads at all for these tests, or would a patch like

-if (threads < 2)
+if (threads < 2 && !GIT_FORCE_PRELOAD_TEST)
 return;

work as well?

That way tests can use any number of threads, though
they currently have no way of overriding the heuristic, yet.

With this alternative patch, it sounds to me as if the
issues kept more orthogonal. (specifically "Do we use preload?"
and "How many threads?". One could imagine that we later
want to introduce GIT_PRELOAD_THREADS for $reasons
and that would go over well in combination with
GIT_FORCE_PRELOAD_TEST)

It seems to be only an internal test variable, such that we do
not need documentation. (Is that worth mentioning in the
commit message?)

The test to make use of this new variable is found
in another patch I presume?

Stefan