Re: [PATCH] list-objects: mark fewer commits as edges for non-shallow clones

2014-12-20 Thread brian m. carlson 
On Thu, Dec 11, 2014 at 11:26:47AM -0800, Junio C Hamano wrote:
 The right approach would be more like allocating one more bit in
 struct rev_info (call that edge_hint_aggressive), give a new option
 --objects-edge-aggressive, and do something like
 
   if (thin) {
   use_internal_rev_list = 1;
   argv_array_push(rp, is_repository_shallow()
   ? --objects-edge-aggressive
 : --objects-edge);
   }
 
 in this codepath?  I'd actually suggest is_repository_shallow()
 detection to happen one level even higher (i.e. make decision at the
 caller of pack-objects) and decide to pass either --thin or
 --thin-aggressive, so that we can make sure that the damage caused
 by fbd4a70 to be limited only to fetches into shallow repository
 with stronger confidence.

Sorry it's taken me so long to get back to this.  Real life keeps
getting in the way.

I think adding --objects-edge-aggressive is the best way forward here
and then applying the patch above.  If we add --thin-aggressive, we push
the problem to a higher level, which will require more code changes and
make the performance regression continue to affect external remote
helpers, which we don't want.  We know what the right decision is here,
so let's just do it.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: Digital signature

Re: [PATCH] list-objects: mark fewer commits as edges for non-shallow clones

2014-12-20 Thread brian m. carlson 
On Thu, Dec 11, 2014 at 05:51:54PM +0700, Duy Nguyen wrote:
 I'm glad it's now working better for you. Out of curiosity, have you
 enabled pack bitmap on this repo? I would expect it to reduce time
 some (or a lot) more, or we would find something to optimize further.

The client performs much, much worse with pack bitmaps.  The server has
bitmaps enabled, but it doesn't have any patches for this issue applied
to it.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: Digital signature

Re: [PATCH] list-objects: mark fewer commits as edges for non-shallow clones

2014-12-11 Thread Duy Nguyen 
On Thu, Dec 11, 2014 at 10:46 AM, brian m. carlson
sand...@crustytoothpaste.net wrote:
 In commit fbd4a70 (list-objects: mark more commits as edges in
 mark_edges_uninteresting - 2013-08-16), we made --thin much more
 aggressive by reading lots more trees.  This produces much smaller packs
 for shallow clones; however, it causes a significant performance
 regression for non-shallow clones with lots of refs (23.322 seconds vs.
 4.785 seconds with 22400 refs).  Limit this extra edge-marking to
 shallow clones to avoid poor performance.

I'm glad it's now working better for you. Out of curiosity, have you
enabled pack bitmap on this repo? I would expect it to reduce time
some (or a lot) more, or we would find something to optimize further.
-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] list-objects: mark fewer commits as edges for non-shallow clones

2014-12-11 Thread Junio C Hamano 
brian m. carlson sand...@crustytoothpaste.net writes:

 In commit fbd4a70 (list-objects: mark more commits as edges in
 mark_edges_uninteresting - 2013-08-16), we made --thin much more
 aggressive by reading lots more trees.  This produces much smaller packs
 for shallow clones; however, it causes a significant performance
 regression for non-shallow clones with lots of refs (23.322 seconds vs.
 4.785 seconds with 22400 refs).  Limit this extra edge-marking to
 shallow clones to avoid poor performance.

This change affects non-clone/fetch uses of object listing depending
on the shallowness of the repository, and does not even care if it
is driven as part of the pack-object codepath, if I am reading it
correctly.  It smells wrong.

The problematic fbd4a70 already had unintended fallout that needed
to be corrected with 200abe74 (list-objects: only look at cmdline
trees with edge_hint, 2014-01-20).  The current code with the fix,
the decision to use the more expensive marking is tied to
edge_hint. I notice that edge_hint is turned on only if the caller
of rev-list passes the --objects-edge option, and currently that
only happens in the pack-objects codepath when thin is given.
Perhaps that part should decide if it really wants to do edge_hint
depending on the shallowness of the repository perhaps?

That is, something like this instead?

 builtin/pack-objects.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 3f9f5c7..a9ebf56 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2709,7 +2709,7 @@ int cmd_pack_objects(int argc, const char **argv, const 
char *prefix)
usage_with_options(pack_usage, pack_objects_options);
 
argv_array_push(rp, pack-objects);
-   if (thin) {
+   if (thin  is_repository_shallow()) {
use_internal_rev_list = 1;
argv_array_push(rp, --objects-edge);
} else
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] list-objects: mark fewer commits as edges for non-shallow clones

2014-12-11 Thread Junio C Hamano 
Junio C Hamano gits...@pobox.com writes:

 This change affects non-clone/fetch uses of object listing depending
 on the shallowness of the repository, and does not even care if it
 is driven as part of the pack-object codepath, if I am reading it
 correctly.  It smells wrong.

 The problematic fbd4a70 already had unintended fallout that needed
 to be corrected with 200abe74 (list-objects: only look at cmdline
 trees with edge_hint, 2014-01-20).  The current code with the fix,
 the decision to use the more expensive marking is tied to
 edge_hint. I notice that edge_hint is turned on only if the caller
 of rev-list passes the --objects-edge option, and currently that
 only happens in the pack-objects codepath when thin is given.
 Perhaps that part should decide if it really wants to do edge_hint
 depending on the shallowness of the repository perhaps?

 That is, something like this instead?

Eh, perhaps not like that, as that would disable milder use of
thin when fetching into non-shallow repository.

The right approach would be more like allocating one more bit in
struct rev_info (call that edge_hint_aggressive), give a new option
--objects-edge-aggressive, and do something like

if (thin) {
use_internal_rev_list = 1;
argv_array_push(rp, is_repository_shallow()
? --objects-edge-aggressive
: --objects-edge);
}

in this codepath?  I'd actually suggest is_repository_shallow()
detection to happen one level even higher (i.e. make decision at the
caller of pack-objects) and decide to pass either --thin or
--thin-aggressive, so that we can make sure that the damage caused
by fbd4a70 to be limited only to fetches into shallow repository
with stronger confidence.



  builtin/pack-objects.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
 index 3f9f5c7..a9ebf56 100644
 --- a/builtin/pack-objects.c
 +++ b/builtin/pack-objects.c
 @@ -2709,7 +2709,7 @@ int cmd_pack_objects(int argc, const char **argv, const 
 char *prefix)
   usage_with_options(pack_usage, pack_objects_options);
  
   argv_array_push(rp, pack-objects);
 - if (thin) {
 + if (thin  is_repository_shallow()) {
   use_internal_rev_list = 1;
   argv_array_push(rp, --objects-edge);
   } else
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] list-objects: mark fewer commits as edges for non-shallow clones

2014-12-10 Thread brian m. carlson 
In commit fbd4a70 (list-objects: mark more commits as edges in
mark_edges_uninteresting - 2013-08-16), we made --thin much more
aggressive by reading lots more trees.  This produces much smaller packs
for shallow clones; however, it causes a significant performance
regression for non-shallow clones with lots of refs (23.322 seconds vs.
4.785 seconds with 22400 refs).  Limit this extra edge-marking to
shallow clones to avoid poor performance.

Suggested-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
Signed-off-by: brian m. carlson sand...@crustytoothpaste.net
---
 list-objects.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/list-objects.c b/list-objects.c
index 2910bec..274ebb4 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -151,13 +151,14 @@ void mark_edges_uninteresting(struct rev_info *revs, 
show_edge_fn show_edge)
 {
struct commit_list *list;
int i;
+   int shallow = is_repository_shallow();
 
for (list = revs-commits; list; list = list-next) {
struct commit *commit = list-item;
 
if (commit-object.flags  UNINTERESTING) {
mark_tree_uninteresting(commit-tree);
-   if (revs-edge_hint  !(commit-object.flags  SHOWN)) 
{
+   if (shallow  revs-edge_hint  
!(commit-object.flags  SHOWN)) {
commit-object.flags |= SHOWN;
show_edge(commit);
}
@@ -165,6 +166,8 @@ void mark_edges_uninteresting(struct rev_info *revs, 
show_edge_fn show_edge)
}
mark_edge_parents_uninteresting(commit, revs, show_edge);
}
+   if (!shallow)
+   return;
if (revs-edge_hint) {
for (i = 0; i  revs-cmdline.nr; i++) {
struct object *obj = revs-cmdline.rev[i].item;
-- 
2.2.0.rc0.207.ga3a616c

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html