diff mbox

[libiberty] add obstack macros (was Re: PR #53525 - track-macro-expansion performance regression)

Message ID alpine.LNX.2.02.1207312009120.20463@localhost.localdomain
State New
Headers show

Commit Message

Dimitrios Apostolou Aug. 4, 2012, 12:06 a.m. UTC 
Hi Dodji, I appreciate your review but I'm replying for now only for the 
libiberty.h part (attached updated patch), which I've needed elsewhere and 
seems the easiest to quickly apply.

On Wed, 18 Jul 2012, Dodji Seketeli wrote:
>
>> === modified file 'include/libiberty.h'
>> --- include/libiberty.h	2011-09-28 19:04:30 +0000
>> +++ include/libiberty.h	2012-07-07 16:04:01 +0000
>
> [...]
>
>> -/* Type-safe obstack allocator.  */
>> +/* Type-safe obstack allocator. You must first initialize the obstack with
>> +   obstack_init() or _obstack_begin(). */
>
> Why not just using the _obstack_begin function?  Why introducing the
> use of the obstack_init macro?

Please correct me if I'm wrong, but my impression is that obstack_init
is the documented way (see [1]), _obstack_begin seems hidden and gives 
access to dangerous (performance-wise) parameters, even though we always 
use it with the defaults (hmmm double checking I see that we mostly 
set size 0 which is default, but also alignment 0???).

[1] http://www.gnu.org/software/libc/manual/html_node/Preparing-for-Obstacks.html


>> -#define XOBFINISH(O, T)         ((T) obstack_finish ((O)))
>
> I think this change is not needed.  You basically remove this line to
> replace it with the hunk below, that comes later in the patch:
>
>> +#define XOBFINISH(O, PT)	((PT) obstack_finish ((O)))
>
> So I believe you could do away with the change.
>

I just wanted to point out that it expects and returns a Pointer to the 
Type, e.g. after you XOBGROW(O, int, DATA) you should XOBFINISH(O, int *).
It would probably be better for XOBFINISH to accept T and return (T *), 
but I didn't want to change existing status quo.

I think I addressed your other concerns (style, comments, unneeded macro 
params). CC'd maintainers. What do you think about attached patch?


Thanks,
Dimitris


P.S. Personally I prefer the original obstack interface, i.e. 
obstack_grow(), obstack_finish() etc. I just added the macros to follow 
existing style and avoid typecasting because of C++. Let me know if it's 
OK to use obstacks directly and I'll back out the patch.




2012-08-04 Dimitrios Apostolou <jimis@gmx.net>

 	* libiberty.h
 	(XOBDELETE,XOBGROW,XOBGROWVEC,XOBSHRINK,XOBSHRINKVEC): New
 	type-safe macros for obstack allocation.
 	(XOBFINISH): Renamed argument to PT since it is a pointer to T.

Comments

Ian Lance Taylor Aug. 4, 2012, 12:26 a.m. UTC | #1 
> 2012-08-04 Dimitrios Apostolou <jimis@gmx.net>
>
>         * libiberty.h
>         (XOBDELETE,XOBGROW,XOBGROWVEC,XOBSHRINK,XOBSHRINKVEC): New
>         type-safe macros for obstack allocation.
>         (XOBFINISH): Renamed argument to PT since it is a pointer to T.

> +/* Type-safe obstack allocator. You must first initialize the obstack with
> +   obstack_init() or _obstack_begin().

This should recommend obstack_init, or obstack_begin, but not
_obstack_begin.  Also obstack_specify_allocation and
obstack_specify_allocation_with_arg are OK, so really it might be
better not to list the functions, but simply say "You must first
initialization the obstack."

> +   T: Type,  O: Obstack,  N: Number of elements,  S: raw Size,

s/Size/size/

> +#define XOBSHRINK(O, T)		obstack_blank ((O), -1 * sizeof (T))
> +#define XOBSHRINKVEC(O, T, N)	obstack_blank ((O), -1 * sizeof (T) * (N))

These are hard to use safely.  I'm not sure we should define them at all.

> +#define XOBFINISH(O, PT)	((PT) obstack_finish ((O)))

For XOBNEW, etc., we use (T *) rather than (PT).  Using (PT) seems
error-probe--it's the only use of the obstack with a different type
parameter.  Why not use T rather than PT here, and return (T *)?

Ian
Dimitrios Apostolou Aug. 4, 2012, 4:40 p.m. UTC | #2 
On Fri, 3 Aug 2012, Ian Lance Taylor wrote:

>> 2012-08-04 Dimitrios Apostolou <jimis@gmx.net>
>>
>>         * libiberty.h
>>         (XOBDELETE,XOBGROW,XOBGROWVEC,XOBSHRINK,XOBSHRINKVEC): New
>>         type-safe macros for obstack allocation.
>>         (XOBFINISH): Renamed argument to PT since it is a pointer to T.
>
>> +/* Type-safe obstack allocator. You must first initialize the obstack with
>> +   obstack_init() or _obstack_begin().
>
> This should recommend obstack_init, or obstack_begin, but not
> _obstack_begin.  Also obstack_specify_allocation and
> obstack_specify_allocation_with_arg are OK, so really it might be
> better not to list the functions, but simply say "You must first
> initialization the obstack."

Grep reveals that 9 out of 10 times we use _obstack_begin(), and we set 
alignment to 0 (isn't it suboptimal?).

>
>> +   T: Type,  O: Obstack,  N: Number of elements,  S: raw Size,
>
> s/Size/size/
>
>> +#define XOBSHRINK(O, T)		obstack_blank ((O), -1 * sizeof (T))
>> +#define XOBSHRINKVEC(O, T, N)	obstack_blank ((O), -1 * sizeof (T) * (N))
>
> These are hard to use safely.  I'm not sure we should define them at all.

I've already used XOBSHRINK and it looks clear to me, but I could use 
obstack_blank() directly if necessary.

>
>> +#define XOBFINISH(O, PT)	((PT) obstack_finish ((O)))
>
> For XOBNEW, etc., we use (T *) rather than (PT).  Using (PT) seems
> error-probe--it's the only use of the obstack with a different type
> parameter.  Why not use T rather than PT here, and return (T *)?

I'd have to change many (about 60) occurences of XOBFINISH if I change 
that. I'd go for it if I was sure it's what we want, it can be a separate 
patch later on.


Thanks,
Dimitris
Ian Lance Taylor Aug. 5, 2012, 4:37 a.m. UTC | #3 
On Sat, Aug 4, 2012 at 9:40 AM, Dimitrios Apostolou <jimis@gmx.net> wrote:
> On Fri, 3 Aug 2012, Ian Lance Taylor wrote:
>
>>> 2012-08-04 Dimitrios Apostolou <jimis@gmx.net>
>>>
>>>         * libiberty.h
>>>         (XOBDELETE,XOBGROW,XOBGROWVEC,XOBSHRINK,XOBSHRINKVEC): New
>>>         type-safe macros for obstack allocation.
>>>         (XOBFINISH): Renamed argument to PT since it is a pointer to T.
>>
>>
>>> +/* Type-safe obstack allocator. You must first initialize the obstack
>>> with
>>> +   obstack_init() or _obstack_begin().
>>
>>
>> This should recommend obstack_init, or obstack_begin, but not
>> _obstack_begin.  Also obstack_specify_allocation and
>> obstack_specify_allocation_with_arg are OK, so really it might be
>> better not to list the functions, but simply say "You must first
>> initialization the obstack."
>
>
> Grep reveals that 9 out of 10 times we use _obstack_begin(), and we set
> alignment to 0 (isn't it suboptimal?).

I'm not sure where you are looking.  I only see one call to
_obstack_begin in the gcc directory, and it could easily be replaced
with a call to obstack_specify_allocation instead.  It does set the
alignment to 0, but that just directs the obstack code to use the
default alignment, which is the alignment of double.  I think that
should normally be fine.


>>> +   T: Type,  O: Obstack,  N: Number of elements,  S: raw Size,
>>
>>
>> s/Size/size/
>>
>>> +#define XOBSHRINK(O, T)                obstack_blank ((O), -1 * sizeof
>>> (T))
>>> +#define XOBSHRINKVEC(O, T, N)  obstack_blank ((O), -1 * sizeof (T) *
>>> (N))
>>
>>
>> These are hard to use safely.  I'm not sure we should define them at all.
>
>
> I've already used XOBSHRINK and it looks clear to me, but I could use
> obstack_blank() directly if necessary.

I'm a bit concerned because they only work if space has already been
allocated, and there is no checking.  Also I only see them used in
genautomata.c.  But I guess it's OK.


>>> +#define XOBFINISH(O, PT)       ((PT) obstack_finish ((O)))
>>
>>
>> For XOBNEW, etc., we use (T *) rather than (PT).  Using (PT) seems
>> error-probe--it's the only use of the obstack with a different type
>> parameter.  Why not use T rather than PT here, and return (T *)?
>
>
> I'd have to change many (about 60) occurences of XOBFINISH if I change that.
> I'd go for it if I was sure it's what we want, it can be a separate patch
> later on.

I'm sorry to ask you to change a lot of code, but it simply doesn't
make sense to me to have all but one macro take the type as an
argument, and have one macro take a pointer to the type.  They really
have to be consistent.

Ian
Dimitrios Apostolou Aug. 5, 2012, 6:44 p.m. UTC | #4 
On Sat, 4 Aug 2012, Ian Lance Taylor wrote:

>> On Fri, 3 Aug 2012, Ian Lance Taylor wrote:
>
> I'm not sure where you are looking.  I only see one call to
> _obstack_begin in the gcc directory, and it could easily be replaced
> with a call to obstack_specify_allocation instead.

In libcpp/ mostly, but only 4 cases so that's not too many, I can change 
those with obstack_init/begin.

>>>> +#define XOBFINISH(O, PT)       ((PT) obstack_finish ((O)))
>>>
>>>
>>> For XOBNEW, etc., we use (T *) rather than (PT).  Using (PT) seems
>>> error-probe--it's the only use of the obstack with a different type
>>> parameter.  Why not use T rather than PT here, and return (T *)?
>>
>>
>> I'd have to change many (about 60) occurences of XOBFINISH if I change that.
>> I'd go for it if I was sure it's what we want, it can be a separate patch
>> later on.
>
> I'm sorry to ask you to change a lot of code, but it simply doesn't
> make sense to me to have all but one macro take the type as an
> argument, and have one macro take a pointer to the type.  They really
> have to be consistent.

No problem, if anyone else doesn't object I'll change those (in a second 
patch, right?).


Thanks,
Dimitris
Ian Lance Taylor Aug. 6, 2012, 3:12 a.m. UTC | #5 
On Sun, Aug 5, 2012 at 11:44 AM, Dimitrios Apostolou <jimis@gmx.net> wrote:
>
> No problem, if anyone else doesn't object I'll change those (in a second
> patch, right?).

Can you please resend the patch you propose to check into mainline?  Thanks.

Ian
diff mbox

Patch

=== modified file 'include/libiberty.h'
--- include/libiberty.h	2011-09-28 19:04:30 +0000
+++ include/libiberty.h	2012-08-03 23:51:55 +0000
@@ -361,12 +361,21 @@  extern unsigned int xcrc32 (const unsign
 #define XDUPVAR(T, P, S1, S2)	((T *) xmemdup ((P), (S1), (S2)))
 #define XRESIZEVAR(T, P, S)	((T *) xrealloc ((P), (S)))
 
-/* Type-safe obstack allocator.  */
+/* Type-safe obstack allocator. You must first initialize the obstack with
+   obstack_init() or _obstack_begin().
+   T: Type,  O: Obstack,  N: Number of elements,  S: raw Size,
+   PT: Pointer to T,  D: Data,  P: Pointer to element.  */
 
 #define XOBNEW(O, T)		((T *) obstack_alloc ((O), sizeof (T)))
 #define XOBNEWVEC(O, T, N)	((T *) obstack_alloc ((O), sizeof (T) * (N)))
 #define XOBNEWVAR(O, T, S)	((T *) obstack_alloc ((O), (S)))
-#define XOBFINISH(O, T)         ((T) obstack_finish ((O)))
+#define XOBDELETE(O, P)		(obstack_free ((O), (P)))
+
+#define XOBGROW(O, T, D)	obstack_grow ((O), (D), sizeof (T))
+#define XOBGROWVEC(O, T, D, N)	obstack_grow ((O), (D), sizeof (T) * (N))
+#define XOBSHRINK(O, T)		obstack_blank ((O), -1 * sizeof (T))
+#define XOBSHRINKVEC(O, T, N)	obstack_blank ((O), -1 * sizeof (T) * (N))
+#define XOBFINISH(O, PT)	((PT) obstack_finish ((O)))
 
 /* hex character manipulation routines */