Thread: refactor heap_deform_tuple guts

refactor heap_deform_tuple guts

From
Alvaro Herrera
Date:
Hi,

heap_deform_tuple and slot_deform_tuple contain duplicated code.  This
patch refactors them so that the guts are in a single place.

I have checked the resulting assembly code for heap_deform_tuple, and
with the "inline" declaration, the gcc version I have (4.7.2) generates
almost identical output both after the patch than before, thus there
shouldn't be any slowdown.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Attachment

Re: refactor heap_deform_tuple guts

From
Robert Haas
Date:
On Tue, Aug 6, 2013 at 6:32 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> heap_deform_tuple and slot_deform_tuple contain duplicated code.  This
> patch refactors them so that the guts are in a single place.
>
> I have checked the resulting assembly code for heap_deform_tuple, and
> with the "inline" declaration, the gcc version I have (4.7.2) generates
> almost identical output both after the patch than before, thus there
> shouldn't be any slowdown.

Although I'm generally in favor of eliminating duplicated code, I have
to admit that in this case I'm not sure I see the point.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Re: refactor heap_deform_tuple guts

From
Alvaro Herrera
Date:
Robert Haas escribió:
> On Tue, Aug 6, 2013 at 6:32 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > heap_deform_tuple and slot_deform_tuple contain duplicated code.  This
> > patch refactors them so that the guts are in a single place.
> >
> > I have checked the resulting assembly code for heap_deform_tuple, and
> > with the "inline" declaration, the gcc version I have (4.7.2) generates
> > almost identical output both after the patch than before, thus there
> > shouldn't be any slowdown.
> 
> Although I'm generally in favor of eliminating duplicated code, I have
> to admit that in this case I'm not sure I see the point.

Yeah, I guess in isolation this doesn't make that much sense.  I am
hesitant to create a third copy in the minmax patch, but I will do that
for now and propose the refactoring later.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services



Re: refactor heap_deform_tuple guts

From
Andres Freund
Date:
On 2013-08-07 10:36:52 -0400, Alvaro Herrera wrote:
> Robert Haas escribió:
> > On Tue, Aug 6, 2013 at 6:32 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > > heap_deform_tuple and slot_deform_tuple contain duplicated code.  This
> > > patch refactors them so that the guts are in a single place.
> > >
> > > I have checked the resulting assembly code for heap_deform_tuple, and
> > > with the "inline" declaration, the gcc version I have (4.7.2) generates
> > > almost identical output both after the patch than before, thus there
> > > shouldn't be any slowdown.
> > 
> > Although I'm generally in favor of eliminating duplicated code, I have
> > to admit that in this case I'm not sure I see the point.
> 
> Yeah, I guess in isolation this doesn't make that much sense.  I am
> hesitant to create a third copy in the minmax patch, but I will do that
> for now and propose the refactoring later.

Well, you didn't mention upthread that you want to do this because
you're going to need another variant of the same code. Imo that's
sufficient reasoning.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services



Re: refactor heap_deform_tuple guts

From
Alvaro Herrera
Date:
Andres Freund escribió:
> On 2013-08-07 10:36:52 -0400, Alvaro Herrera wrote:

> > Yeah, I guess in isolation this doesn't make that much sense.  I am
> > hesitant to create a third copy in the minmax patch, but I will do that
> > for now and propose the refactoring later.
> 
> Well, you didn't mention upthread that you want to do this because
> you're going to need another variant of the same code. Imo that's
> sufficient reasoning.

Well, I would need to tweak the new code again, moving it from
heaptuple.c to htup_details.h as a STATIC_IF_INLINE function; and
furthermore I have noticed that if I modify the code in minmax, I can
have it do one more thing that I would need to do outside of it anyway.
(These tuples have two null bitmaps; I would have to decode the second
one separately.  Doing it inside the new "deform" variant is much
easier.)

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services