On 2015-08-05 15:46:36 +0200, Andres Freund wrote:
> On 2015-08-05 15:08:29 +0200, Andres Freund wrote:
> > We might later want to change some of the harder to maintain macros to
> > inline functions, but that seems better done separately.
>
> Here's a conversion for fastgetattr() and heap_getattr(). Not only is
> the resulting code significantly more readable, but the conversion also
> shrinks the code size:
>
> $ ls -l src/backend/postgres_stock src/backend/postgres
> -rwxr-xr-x 1 andres andres 37054832 Aug 5 15:18 src/backend/postgres_stock
> -rwxr-xr-x 1 andres andres 37209288 Aug 5 15:23 src/backend/postgres
>
> $ size src/backend/postgres_stock src/backend/postgres
> text data bss dec hex filename
> 7026843 49982 298584 7375409 708a31 src/backend/postgres_stock
> 7023851 49982 298584 7372417 707e81 src/backend/postgres
>
> stock is the binary compiled without the conversion.
>
> A lot of the size difference is debugging information (which now needs
> less duplicated information on each callsite), but you can see that the
> text section (the actual executable code) shrank by 3k.
>
> stripping the binary shows exactly that:
> -rwxr-xr-x 1 andres andres 7076760 Aug 5 15:44 src/backend/postgres_s
> -rwxr-xr-x 1 andres andres 7079512 Aug 5 15:45 src/backend/postgres_stock_s
>
> To be sure this doesn't cause problems I ran a readonly pgbench. There's
> a very slight, but reproducible, performance benefit. I don't think
> that's a reason for the change, I just wanted to make sure there's no
> regressions.
Slightly updated version attached. The only changes are updates to some
comments referencing the 'fastgetattr macro' and the like. Oh, and an
additional newline.
In my opinion this drastically increases readability and thus should be
applied. Will do so sometime tomorrow unless there's protest.
Btw, I found that many changes are much more readable when changing
git's config to use histogramm diffs (git config --global diff.algorithm
histogram or --histogram).
Regards,
Andres
>From db76501e9c987c9a0b847ea8a2256a1ec9150b37 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Tue, 11 Aug 2015 13:02:45 +0200
Subject: [PATCH] Convert fastgetattr() and heap_getattr() into inline
functions.
The current macro is very hard to read. Now that we can unconditionally
use inline functions there's no reason to keep them that way.
In my gcc 5 based environment this shaves of nearly 200k of the final
binary size. A lot of that is debugging information, but the
.text (i.e. code) section alone shrinks by 3k.
Discussion: 20150805134636.gf12...@awork2.anarazel.de
---
src/backend/access/common/heaptuple.c | 6 +-
src/backend/access/heap/heapam.c | 46 ----------
src/include/access/htup_details.h | 160 ++++++++++++++++------------------
3 files changed, 78 insertions(+), 134 deletions(-)
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index 045e0a7..017c551 100644
--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -328,8 +328,8 @@ heap_attisnull(HeapTuple tup, int attnum)
/* ----------------
* nocachegetattr
*
- * This only gets called from fastgetattr() macro, in cases where
- * we can't use a cacheoffset and the value is not null.
+ * This only gets called from fastgetattr(), in cases where we can't use
+ * a cacheoffset and the value is not null.
*
* This caches attribute offsets in the attribute descriptor.
*
@@ -544,7 +544,7 @@ nocachegetattr(HeapTuple tuple,
*
* Fetch the value of a system attribute for a tuple.
*
- * This is a support routine for the heap_getattr macro. The macro
+ * This is a support routine for the heap_getattr. The inline function
* has already determined that the attnum refers to a system attribute.
* ----------------
*/
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 3701d8e..ff93b3c 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -983,52 +983,6 @@ heapgettup_pagemode(HeapScanDesc scan,
}
-#if defined(DISABLE_COMPLEX_MACRO)
-/*
- * This is formatted so oddly so that the correspondence to the macro
- * definition in access/htup_details.h is maintained.
- */
-Datum
-fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
- bool *isnull)
-{
- return (
- (attnum) > 0 ?
- (
- (*(isnull) = false),
- HeapTupleNoNulls(tup) ?
- (
- (tupleDesc)->attrs[(attnum) - 1]->attcacheoff >= 0 ?
- (
- fetchatt((tupleDesc)->attrs[(attnum) - 1],
- (char *) (tup)->t_data + (tup)->t_data->t_hoff +
- (tupleDesc)->attrs[(attnum) - 1]->attcacheoff)
- )
- :
- nocachegetattr((tup), (attnum), (tupleDesc))
- )
- :
- (
- att_isnull((attnum) - 1, (tup)->t_data->t_bits) ?
- (
- (*(isnull) = true),
- (Datum) NULL
- )
- :
- (
- nocachegetattr((tup), (attnum), (tupleDesc))
- )
- )
- )
- :
- (
- (Datum) NULL
- )
- );
-}
-#endif /* defined(DISABLE_COMPLEX_MACRO) */
-
-
/* ----------------------------------------------------------------
* heap access method interface
* ----------------------------------------------------------------
diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h
index 8dd530bd..2f20b07 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -673,91 +673,6 @@ struct MinimalTupleData
#define HeapTupleSetOid(tuple, oid) \
HeapTupleHeaderSetOid((tuple)->t_data, (oid))
-
-/* ----------------
- * fastgetattr
- *
- * Fetch a user attribute's value as a Datum (might be either a
- * value, or a pointer into the data area of the tuple).
- *
- * This must not be used when a system attribute might be requested.
- * Furthermore, the passed attnum MUST be valid. Use heap_getattr()
- * instead, if in doubt.
- *
- * This gets called many times, so we macro the cacheable and NULL
- * lookups, and call nocachegetattr() for the rest.
- * ----------------
- */
-
-#if !defined(DISABLE_COMPLEX_MACRO)
-
-#define fastgetattr(tup, attnum, tupleDesc, isnull) \
-( \
- AssertMacro((attnum) > 0), \
- (*(isnull) = false), \
- HeapTupleNoNulls(tup) ? \
- ( \
- (tupleDesc)->attrs[(attnum)-1]->attcacheoff >= 0 ? \
- ( \
- fetchatt((tupleDesc)->attrs[(attnum)-1], \
- (char *) (tup)->t_data + (tup)->t_data->t_hoff + \
- (tupleDesc)->attrs[(attnum)-1]->attcacheoff) \
- ) \
- : \
- nocachegetattr((tup), (attnum), (tupleDesc)) \
- ) \
- : \
- ( \
- att_isnull((attnum)-1, (tup)->t_data->t_bits) ? \
- ( \
- (*(isnull) = true), \
- (Datum)NULL \
- ) \
- : \
- ( \
- nocachegetattr((tup), (attnum), (tupleDesc)) \
- ) \
- ) \
-)
-#else /* defined(DISABLE_COMPLEX_MACRO) */
-
-extern Datum fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
- bool *isnull);
-#endif /* defined(DISABLE_COMPLEX_MACRO) */
-
-
-/* ----------------
- * heap_getattr
- *
- * Extract an attribute of a heap tuple and return it as a Datum.
- * This works for either system or user attributes. The given attnum
- * is properly range-checked.
- *
- * If the field in question has a NULL value, we return a zero Datum
- * and set *isnull == true. Otherwise, we set *isnull == false.
- *
- * <tup> is the pointer to the heap tuple. <attnum> is the attribute
- * number of the column (field) caller wants. <tupleDesc> is a
- * pointer to the structure describing the row and all its fields.
- * ----------------
- */
-#define heap_getattr(tup, attnum, tupleDesc, isnull) \
- ( \
- ((attnum) > 0) ? \
- ( \
- ((attnum) > (int) HeapTupleHeaderGetNatts((tup)->t_data)) ? \
- ( \
- (*(isnull) = true), \
- (Datum)NULL \
- ) \
- : \
- fastgetattr((tup), (attnum), (tupleDesc), (isnull)) \
- ) \
- : \
- heap_getsysattr((tup), (attnum), (tupleDesc), (isnull)) \
- )
-
-
/* prototypes for functions in common/heaptuple.c */
extern Size heap_compute_data_size(TupleDesc tupleDesc,
Datum *values, bool *isnull);
@@ -790,4 +705,79 @@ extern MinimalTuple heap_copy_minimal_tuple(MinimalTuple mtup);
extern HeapTuple heap_tuple_from_minimal_tuple(MinimalTuple mtup);
extern MinimalTuple minimal_tuple_from_heap_tuple(HeapTuple htup);
+
+/* ----------------
+ * fastgetattr
+ *
+ * Fetch a user attribute's value as a Datum (might be either a
+ * value, or a pointer into the data area of the tuple).
+ *
+ * This must not be used when a system attribute might be requested.
+ * Furthermore, the passed attnum MUST be valid. Use heap_getattr()
+ * instead, if in doubt.
+ *
+ * This gets called many times, so we inline the cacheable and NULL
+ * lookups, and call nocachegetattr() for the rest.
+ * ----------------
+ */
+static inline Datum
+fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
+ bool *isnull)
+{
+ AssertArg(attnum > 0);
+
+ *isnull = false;
+
+ if (HeapTupleNoNulls(tup))
+ {
+ Form_pg_attribute attr = tupleDesc->attrs[attnum - 1];
+
+ if (attr->attcacheoff >= 0)
+ {
+ return fetchatt(attr,
+ (char *) tup->t_data +
+ tup->t_data->t_hoff +
+ attr->attcacheoff);
+ }
+ /* fall through to nocachegetattr */
+ }
+ else if (att_isnull(attnum - 1, tup->t_data->t_bits))
+ {
+ *isnull = true;
+ return (Datum) NULL;
+ }
+
+ return nocachegetattr(tup, attnum, tupleDesc);
+}
+
+/* ----------------
+ * heap_getattr
+ *
+ * Extract an attribute of a heap tuple and return it as a Datum.
+ * This works for either system or user attributes. The given attnum
+ * is properly range-checked.
+ *
+ * If the field in question has a NULL value, we return a zero Datum
+ * and set *isnull == true. Otherwise, we set *isnull == false.
+ * ----------------
+ */
+static inline Datum
+heap_getattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
+ bool *isnull)
+{
+ if (attnum > 0)
+ {
+ if (attnum > HeapTupleHeaderGetNatts((tup)->t_data))
+ {
+ *isnull = true;
+ return (Datum) NULL;
+ }
+ else
+ return fastgetattr(tup, attnum, tupleDesc, isnull);
+ }
+
+ return heap_getsysattr(tup, attnum, tupleDesc, isnull);
+}
+
+
#endif /* HTUP_DETAILS_H */
--
2.3.0.149.gf3f4077.dirty
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
