From: Matthew Fernandez Date: Thu, 15 Dec 2022 16:49:04 +0000 (-0800) Subject: cgraph agxbuf: expand usable inline storage X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=3a00bf0423dcf01cc8b0e071cb7f5cadbd441542;p=graphviz cgraph agxbuf: expand usable inline storage As indicated by the changes to the layout diagram in this diff, previously we were essentially wasting a number of bytes (3 on x86, 7 on x86-64) on structure padding. Through some rearrangement and packing, we can make these bytes usable for the inline storage area. The definition of the struct is not quite what you might expect to see. The `located` field is in the first member of the union but is actually used regardless of what mode an `agxbuf` is in (heap, stack, inline). This is necessary to get the desired packing. It does not overlap with other struct fields, though reading and writing a struct through different union members is well defined in C99. This optimization might seem a little niche. But `agxbuf` objects are extensively used. Once inline `agxbuf` objects become possible (an upcoming commit), this saves us heap allocations in numerous scenarios. Gitlab: #2302 --- diff --git a/lib/cgraph/agxbuf.h b/lib/cgraph/agxbuf.h index 6970add63..4c812d1a5 100644 --- a/lib/cgraph/agxbuf.h +++ b/lib/cgraph/agxbuf.h @@ -12,6 +12,7 @@ #include #include +#include #include #include #include @@ -33,11 +34,13 @@ typedef enum { /// /// This has the following layout assuming x86-64. /// -/// ┌───────────────┬───────────────┬───────────────┬───────────────┐ -/// │ buf │ size │ capacity │ │ -/// ├───────────────┴───────────────┴───────────────┤ located │ -/// │ store │ │ -/// └───────────────────────────────────────────────┴───────────────┘ +/// located +/// ↓ +/// ┌───────────────┬───────────────┬───────────────┬─────────────┬─┐ +/// │ buf │ size │ capacity │ padding │ │ +/// ├───────────────┴───────────────┴───────────────┴─────────────┼─┤ +/// │ store │ │ +/// └─────────────────────────────────────────────────────────────┴─┘ /// 0 8 16 24 32 /// /// \p buf, \p size, and \p capacity are in use when \p located is @@ -46,15 +49,17 @@ typedef enum { typedef struct { union { struct { - char *buf; ///< start of buffer - size_t size; ///< number of characters in the buffer - size_t capacity; ///< available bytes in the buffer + char *buf; ///< start of buffer + size_t size; ///< number of characters in the buffer + size_t capacity; ///< available bytes in the buffer + char padding[sizeof(size_t) - 1]; ///< unused; for alignment + unsigned char + located; ///< where does the backing memory for this buffer live? }; - char store[sizeof(char *) + - sizeof(size_t) * 2]; ///< inline storage used when \p located is - ///< > \p AGXBUF_ON_STACK + char store[sizeof(char *) + sizeof(size_t) * 3 - + 1]; ///< inline storage used when \p located is + ///< > \p AGXBUF_ON_STACK }; - size_t located; ///< where does the backing memory for this buffer live? } agxbuf; static inline bool agxbuf_is_inline(const agxbuf *xb) { @@ -228,7 +233,8 @@ static inline PRINTF_LIKE(2, 3) int agxbprint(agxbuf *xb, const char *fmt, assert(result == (int)(size - 1) || result < 0); if (result > 0) { if (agxbuf_is_inline(xb)) { - xb->located += (size_t)result; + assert(result <= (int)UCHAR_MAX); + xb->located += (unsigned char)result; assert(agxblen(xb) <= sizeof(xb->store) && "agxbuf corruption"); } else { xb->size += (size_t)result; @@ -253,7 +259,8 @@ static inline size_t agxbput_n(agxbuf *xb, const char *s, size_t ssz) { size_t len = agxblen(xb); if (agxbuf_is_inline(xb)) { memcpy(&xb->store[len], s, ssz); - xb->located += ssz; + assert(ssz <= UCHAR_MAX); + xb->located += (unsigned char)ssz; assert(agxblen(xb) <= sizeof(xb->store) && "agxbuf corruption"); } else { memcpy(&xb->buf[len], s, ssz);