On 09/19/12 13:56, Stefan Teleman wrote:
This is a proposed fix for the numpunct facet for stdcxx-1056:


FWIW, here is my feed-back. Please correct me if there is something I did not get right about your proposed patch:


0.  Number of reported race conditions is now 0 (zero).

1.  No memory leaks in stdcxx (there are memory leaks reported in either
     libc or glibc, but there's nothing we can do about these anyway).

2.  This fix preserves perfect forwarding in the _numpunct.h header file.

3.  This fix eliminates code from facet.cpp and locale_body.cpp which
     was creating unnecessary overhead, with the potential of causing
     memory corruption, while providing no discernable benefit.

     More specifically:

     It is not true that there was no eviction policy of cached locales or
     facets in stdcxx. Not only cache eviction code existed, and still exists
     today, but cache cleanups and resizing were performed periodically,
     either when an object's reference count dropped to 0 (zero), or whenever
     the number of cached objects fell below sizeof(cache) / 2.

I think you are referring to `live' cache objects and the code which specifically adjusts the size of the buffer according to the number of `live' locales and/or facets in it. In that respect I would not call that eviction because locales and facets with non-zero reference counters are never evicted.

But anyhoo, this is semantics. Bottom line is the locale/facet buffer management code follows a principle of economy.


     First, the default size of the facets and locales caches was too small:
     it was set to 8. I raised this to 32. A direct consequence of this
     insufficient default size of 8 was that the cache had to resize itself
     very soon after program startup. This cache resize operation consists of:
     allocate memory for a new cache, copy the existing cached objects
     from the old cache to the new cache, and then delete[] the old cache.

The optimal number is subject to debate. Probably Martin can give an insight into the reasons for that number. Why did you pick 32 (or is it 64 in your patch) and not any other? Is it something based on your experience as a user or programmer?


     This is a first unnecessary overhead.

A negligible overhead, IMO. The benefits of maintaining a small memory footprint may be important for some environments. As useful as principles may be, see above.


     Second, and as I mentioned above, whenever the number of cached objects
     fell below sizeof(cache) / 2, the cache resized itself, by performing
     the same sequence of operations as described above.

     This is a second unnecessary overhead.


In this respect you could call every memory allocation and de-allocation is an overhead. Please keep in mind that this resembles the operations performed for any sequence containers; how likely is it for a program to have more locale/facet creation/destruction than strings or vectors mutations?


     Third, cached objects were automatically evicted whenever their reference
     count dropped to 0 (zero). There are two consequences to this eviction
     policy: if the program needs to re-use an object (facet or locale) which
     has been evicted and subsequently destroyed, this object needs then to be
     constructed again later on, and subsequently re-inserted into the cache.
     This, in turn, would trigger a cache resize, followed by copying and
     delete[] of the old cache buffer.

     Object eviction followed by destruction followed by reconstruction is
     a third unnecessary overhead. Re-inserting a re-constructed object into,
     the cache, followed by a potential cache resize involving allocation of
     a new buffer, copying pointers from the old cache to the new cache,
     followed by delete[] of the old cache is a fourth unnecessary overhead.

What you describe is the classical behavior of reference-counted object management. Dead, unreferenced objects are destroyed. The overhead of doing so at each `death' is probably too much; a middle ground approach is to do it at a factor of 2. The alternative, if a bit extreme, is to keep all memory ever allocated in a program around just in case it might be needed again.



     Real-life programs tend to reuse locales and/or facets they have created.
     There is no point in destroying and evicting these objects simply because
     there may be periods when the object isn't referenced at time. The object
     is likely to be needed again, later on.

Could you please elaborate a bit on this? Is this your opinion based on your user and/or programmer experience?


     The fix proposed here eliminates the cache eviction and object destruction
     policy completely. Once created, objects remain in the cache, even though
     they may reside in the cache with no references. This eliminates the
     cache resize / copy / delete[] overhead. It also eliminates the overhead
     of re-constructing an evicted / destroyed object, if it is needed again
     later.

4.  Tests and Analysis Results:

     4.1. SunPro 12.3 / Solaris / SPARC / Race Conditions Test:

     
http://s247136804.onlinehome.us/stdcxx-1056-20120919/22.locale.numpunct.mt.sunpro.solaris-sparc.datarace.er.html/index.html

     4.2. SunPro 12.3 / Solaris / SPARC / Heap and Memory Leaks Test:

     
http://s247136804.onlinehome.us/stdcxx-1056-20120919/22.locale.numpunct.mt.sunpro.solaris-sparc.heapcheck.er.html/index.html

     4.3. SunPro 12.3 / Linux / Intel / Race Conditions Test:

     
http://s247136804.onlinehome.us/stdcxx-1056-20120919/22.locale.numpunct.mt.sunpro.linux-intel.datarace.er.html/index.html

     4.4. SunPro 12.3 / Linux / Intel / Heap and Memory Leaks Test:

     
http://s247136804.onlinehome.us/stdcxx-1056-20120919/22.locale.numpunct.mt.sunpro.linux-intel.heapcheck.er.html/index.html

     4.5. Intel 2013 / Linux / Intel / Race Conditions Test:

     
http://s247136804.onlinehome.us/stdcxx-1056-20120919/22.locale.numpunct.mt.intel.linux.datarace.r007ti3.inspxez

     4.6. Intel 2013 / Linux / Intel / Heap and Memory Leaks Test:

     
http://s247136804.onlinehome.us/stdcxx-1056-20120919/22.locale.numpunct.mt.intel.linux.heapcheck.r008mi1.inspxez

5.  Source code for this fix:

     http://s247136804.onlinehome.us/stdcxx-1056-20120919/_numpunct.h
     http://s247136804.onlinehome.us/stdcxx-1056-20120919/facet.cpp
     http://s247136804.onlinehome.us/stdcxx-1056-20120919/locale_body.cpp
     http://s247136804.onlinehome.us/stdcxx-1056-20120919/punct.cpp

Hey Stefan, are the above also timing the changes?

HTH.

Liviu

Reply via email to