Обсуждение: refactor heap_deform_tuple guts
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
Вложения
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
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
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
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