From: Derrick Stolee <stolee@gmail.com>
To: "René Scharfe" <l.s.r@web.de>, "Git Mailing List" <git@vger.kernel.org>
Cc: Derrick Stolee <dstolee@microsoft.com>,
Junio C Hamano <gitster@pobox.com>,
"brian m. carlson" <sandals@crustytoothpaste.net>
Subject: Re: [PATCH] revision: reallocate TOPO_WALK object flags
Date: Wed, 24 Jun 2020 09:51:01 -0400 [thread overview]
Message-ID: <8e5b6b9f-a778-7b20-2c67-2d5d8ff0d8a0@gmail.com> (raw)
In-Reply-To: <29f8b1fc-fac7-12c6-4bfe-28aed7e709c3@web.de>
On 6/24/2020 9:05 AM, René Scharfe wrote:
> The bit fields in struct object have an unfortunate layout. Here's what
> pahole reports on x86_64 GNU/Linux:
>
> struct object {
> unsigned int parsed:1; /* 0: 0 4 */
> unsigned int type:3; /* 0: 1 4 */
>
> /* XXX 28 bits hole, try to pack */
>
> /* Force alignment to the next boundary: */
> unsigned int :0;
>
> unsigned int flags:29; /* 4: 0 4 */
>
> /* XXX 3 bits hole, try to pack */
>
> struct object_id oid; /* 8 32 */
>
> /* size: 40, cachelines: 1, members: 4 */
> /* sum members: 32 */
> /* sum bitfield members: 33 bits, bit holes: 2, sum bit holes: 31 bits */
> /* last cacheline: 40 bytes */
> };
>
> Notice the 1+3+29=33 bits in bit fields and 28+3=31 bits in holes.
This is certainly unfortunate.
> There are holes inside the flags bit field as well -- while some object
> flags are used for more than one purpose, 22, 23 and 24 are still free.
> Use 23 and 24 instead of 27 and 28 for TOPO_WALK_EXPLORED and
> TOPO_WALK_INDEGREE. This allows us to reduce FLAG_BITS by one so that
> all bitfields combined fit into a single 32-bit slot:
>
> struct object {
> unsigned int parsed:1; /* 0: 0 4 */
> unsigned int type:3; /* 0: 1 4 */
> unsigned int flags:28; /* 0: 4 4 */
> struct object_id oid; /* 4 32 */
>
> /* size: 36, cachelines: 1, members: 4 */
> /* last cacheline: 36 bytes */
> };
>
> With this tight packing the size of struct object is reduced by 10%.
> Other architectures probably benefit as well.
Excellent improvement!
> - * revision.h: 0---------10 15 25----28
> + * revision.h: 0---------10 15 23------26
> * fetch-pack.c: 01
> * negotiator/default.c: 2--5
> * walker.c: 0-2
> @@ -79,7 +79,7 @@ struct object_array {
> * builtin/show-branch.c: 0-------------------------------------------26
The only collision is here in builtin/show-branch.c, and when I added
the TOPO_WALK_* bits, I didn't understand that these bits were
sufficiently independent from the topo-walk logic that we could add
collisions here.
I think I realized this behavior when doing the commit-reach.c refactor,
but I never revisited these flag values.
> * builtin/unpack-objects.c: 2021
> */
> -#define FLAG_BITS 29
> +#define FLAG_BITS 28
>
> /*
> * The object type is stored in 3 bits.
> diff --git a/revision.h b/revision.h
> index 93491b79d4..f412ae85eb 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -37,6 +37,10 @@
>
> /* WARNING: This is also used as REACHABLE in commit-graph.c. */
> #define PULL_MERGE (1u<<15)
> +
> +#define TOPO_WALK_EXPLORED (1u<<23)
> +#define TOPO_WALK_INDEGREE (1u<<24)
> +
> /*
> * Indicates object was reached by traversal. i.e. not given by user on
> * command-line or stdin.
> @@ -48,9 +52,6 @@
> #define TRACK_LINEAR (1u<<26)
> #define ALL_REV_FLAGS (((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR | PULL_MERGE)
>
> -#define TOPO_WALK_EXPLORED (1u<<27)
> -#define TOPO_WALK_INDEGREE (1u<<28)
> -
> #define DECORATE_SHORT_REFS 1
> #define DECORATE_FULL_REFS 2
Thankfully, the changes are very simple!
Thanks,
-Stolee
next prev parent reply other threads:[~2020-06-24 13:51 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-24 13:05 [PATCH] revision: reallocate TOPO_WALK object flags René Scharfe
2020-06-24 13:51 ` Derrick Stolee [this message]
2020-06-24 16:06 ` Junio C Hamano
2020-06-24 19:39 ` Jeff King
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=8e5b6b9f-a778-7b20-2c67-2d5d8ff0d8a0@gmail.com \
--to=stolee@gmail.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=l.s.r@web.de \
--cc=sandals@crustytoothpaste.net \
/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).