Re: [PATCH] Add STATIC_GETENV macro.

[OndÅej BÃlka <neleai at seznam dot cz>]

On Sun, Nov 10, 2013 at 06:01:42PM -0500, Carlos O'Donell wrote:
> On 11/10/2013 03:55 AM, OndÅej BÃlka wrote:
> > On Sun, Nov 10, 2013 at 02:52:21AM -0500, Carlos O'Donell wrote:
> >> On 11/09/2013 05:38 AM, OndÅej BÃlka wrote:
> >>> Hi,
> >>>
> >>> This adds a STATIC_GETENV macro for avoiding possible getenv bottlenecks as 
> >>> documented in 'Make getenv O(1)' thread.
> >>>
> >>> As this is useful addition this could be a gnu extension.
> >>>
> >>> An patch that converts internal getenv uses to STATIC_GETENV will
> >>> follow.
> >>>
> >>> There is question if we should make STATIC_GETENV call a secure_getenv
> >>> by default or add STATIC_SECURE_GETENV.
> >>>
> >>> This depends on if getenv calls in next patch could be replaced by
> >>> secure getenv or not.
> >>>
> >>> So far both choices are possible.
> >>>
> >>> 	* stdlib/stdlib.h (STATIC_GETENV): Add.
> >>> 	* manual/startup.texi (Environment Access): Document STATIC_GETENV.
> >>> 	* NEWS: Mention STATIC_GETENV.
> >>
> >> I like where this is going, but new functions like this will need
> >> quite a bit of discussion, and it isn't clear to me what uses users
> >> would use this where they wouldn't just cache the value themselves?
> >>
> > How many will? When writing code getenv/STATIC_GETENV choice is mostly
> > automatic, doing caching less so.
> > 
> > It involves adding static variable and choosing appropriate name, if
> > conditional and if you take standard as seriously as Rich using atomic
> > operations to set result.
> > 
> > Also when one want to refactoring with codebase containing twenty
> > getenvs then with STATIC_GETENV its running a script and checking which
> > parts makes sense.
> > As user it is first checking which parts make sense and then rewrite
> > needed parts which is a repetitive task so when one looks how much he
> > should do he gives up.
> 
> Let me think about this for a while.
> 
> I'm not happy with the name STATIC_GETENV, I wish we could express
> the behaviour rather than implementation as part of the name, but
> that's just my opinion e.g. GETENV_CACHED or something similar.
>
I am ok also with that.

One of future directions is add to library that detect performance
problem a check of how often is getenv called and issue a suggestion to
use GETENV_CACHED or so for callers where it happens more than 10 times.
 
> The other issue is that we must support users compiling programs
> with much older compilers than we support building glibc. When
> were the __sync* builtins added? Can you always rely on them?
> I thought they were being deprecated in favour of the atomic
> builtins? I think you need several ifdefs there to account for
> various ages of compilers.
>
This is bit unclear as they are mentioned in 4.1 documentation but they
are earlier. I did not dig exact date, a first mention of
compare_and_swap in git log is here so we could start from that.

commit 617887919805892874edb154aa8775fecc98641e
Author: rth <rth@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Sun May 7 01:37:07 2000 +0000

As far as atomic builtins are concerned they are a spurious addition, a
check for sync builtin and compatibility code should suffice.

Or as this is new extension we could document minimal required version.