Web lists-archives.com

Re: [PATCH 04/11] pack-objects: use bitfield for object_entry::depth




Nguyễn Thái Ngọc Duy  <pclouds@xxxxxxxxx> writes:

> This does not give us any saving due to padding. But we will be able
> to save once we cut 4 bytes out of this struct in a subsequent patch.
>
> Because of struct packing from now on we can only handle max depth
> 4095 (or even lower when new booleans are added in this struct). This
> should be ok since long delta chain will cause significant slow down
> anyway.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
> ---

This should be marked as RFC/PATCH; I do not have objection to
limiting the delta depth to some reasonable length (rather than the
current 1<<31 or worse 1<<63), and 4k may be such a reasonable limit
(I'd actually think anything more than a few hundreds is probably a
bad idea), but it needs to be documented.

>  builtin/pack-objects.c | 4 ++++
>  pack-objects.h         | 6 ++----
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index a4dbb40824..cfd97da7db 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3068,6 +3068,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  	if (pack_to_stdout != !base_name || argc)
>  		usage_with_options(pack_usage, pack_objects_options);
>  
> +	if (depth > (1 << OE_DEPTH_BITS))
> +		die(_("delta chain depth %d is greater than maximum limit %d"),
> +		    depth, (1 << OE_DEPTH_BITS));
> +
>  	argv_array_push(&rp, "pack-objects");
>  	if (thin) {
>  		use_internal_rev_list = 1;
> diff --git a/pack-objects.h b/pack-objects.h
> index fca334ab4d..3941e6c9a6 100644
> --- a/pack-objects.h
> +++ b/pack-objects.h
> @@ -2,6 +2,7 @@
>  #define PACK_OBJECTS_H
>  
>  #define OE_DFS_STATE_BITS 2
> +#define OE_DEPTH_BITS 12
>  
>  /*
>   * State flags for depth-first search used for analyzing delta cycles.
> @@ -43,10 +44,7 @@ struct object_entry {
>  	unsigned tagged:1; /* near the very tip of refs */
>  	unsigned filled:1; /* assigned write-order */
>  	unsigned dfs_state:OE_DFS_STATE_BITS;
> -
> -	/* XXX 20 bits hole, try to pack */
> -
> -	int depth;
> +	unsigned depth:OE_DEPTH_BITS;
>  
>  	/* size: 120, padding: 4 */
>  };