On Mon, Aug 17, 2015 at 8:44 AM, Mikhail Maltsev <malts...@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
