[PATCH] [ping] Use single shared memory block pool for all pool allocators

[Richard Biener]

On Mon, Aug 17, 2015 at 8:44 AM, Mikhail Maltsev <maltsevm@gmail.com> wrote:
> Hi, all.
> I'm pinging this patch: https://gcc.gnu.org/ml/gcc-patches/2015-08/msg00030.html
>
> And adding some more changes to it (they are not complete yet - so they are more
> like an RFC).
> In this patch I also try to make obstacks use the same block pool as object
> pools. This seems rather easy to implement because obstacks already have
> obstack_chunk_alloc/obstack_chunk_free callbacks, and they are easily replaced.
> I also change the default chunk size to memory_block_pool::block_size. This
> still works for object sizes greater than memory_block_pool::block_size: in this
> case I simply call xmalloc to allocate the requested chunk, and the deallocation
> function uses obstack chunk header to determine chunk's size (and depending on
> it calls either free or memory_block_pool::remove).
> The result is visible on the profile. For example, for tramp3d build
> _obstack_begin is the top caller of xmalloc (it constitutes 107 ms into xmalloc
> self time, which is 375 ms - according to one of my runs). After applying the
> patch it is gone from profile.
> This patch adds new files virtual-memory.h and virtual-memory.cc, which
> currently contain memory_block_pool class and related obstack functions (I plan
> to factor out part of ggc-page.c into this file in order to use
> mmap/VirtualAlloc directly, hence the name "virtual-memory"). Currently this
> file is linked into libcommon, and this seems somewhat wrong to me, because the
> driver and other command line tools don't use all memory management machinery of
> the compiler proper. But obstacks are used by diagnostics.c and this file is
> already in libcommon, so there is probably no easy way to make it use xmalloc
> instead of memory_block_pool::allocate. A possible solution is to create an
> additional file with definitions of mempool_obstack_chunk_alloc/free and use it
> for generators and drivers (or somehow make mempool_obstack_chunk_alloc alias to
> malloc). Do you have any suggestions, how this could be done in a better way?
> Another problem is headers. I included "virtual-memory.h" into coretypes.h,
> because it defines a macro gcc_obstack_init, which now uses functions from
> virtual-memory.h. Alternatively I could just declare those functions in
> coretypes.h. Would that be better?

Well, as coretypes.h makes use of them just via macros you could make sure
to include that file everywhere...  Ok, not really.  I suppose it's fine as-is.

Apart from Richards comments:

+/* Return UNCAST_BLOCK to pool.  */
+inline void
+memory_block_pool::remove (void *uncast_block)
+{
+  block_list *block = reinterpret_cast<block_list *> (uncast_block);
+  block->m_next = instance.m_blocks;
+  instance.m_blocks = block;
+}

you need to use placement new

    new (uncast_block) block_list;

instead of the reinterpret_cast to avoid type-based alias issues (as you
are also inlining this function.

Now some bikeshedding...

+  static inline void *allocate () ATTRIBUTE_MALLOC;
+  static inline void remove (void *);

why is it called 'remove' and not 'release' or 'free'? (ah, release is
already taken)

Also why virtual-memory.{h,cc} and not memory-block.{h,cc}?

I think the patch is ok with the above correctness fix and whatever
choice you take
for the bikeshedding.  Also fixing Richards review comments, of course.

Thanks,
Richard.


> --
> Regards,
>     Mikhail Maltsev