[PATCH] Allocate MCSymbol Name intrusively only when required

[David Blaikie]

If MCSymbol ever ends up with more-than-pointer alignment you'd need to add
padding between the pointer and the MCSymbol - maybe best just to assert
that MCSymbol's alignment <= pointer alignment (& then just use
alignof(NameTy*))?

"getNamePtr" maybe rename to "getNameEntryPtr" and have getNamePtr
functions return NameTy*& rather than NameTy**? (usual prefer reference
over pointer)

& maybe NameTy could be NameEntryTy - NameTy is a bit confusing when it's
not the name mbut a think you can access the name from.

(though perhaps these names have precedence from the types you modeled them
off, I'm not sure)



On Mon, Jun 8, 2015 at 2:06 PM, Pete Cooper <peter_cooper at apple.com> wrote:

> Attached an updated patch with all of the feedback from David fixed.
>
> Cheers,
> Pete
>
>
> On Jun 8, 2015, at 2:02 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com>
> wrote:
>
>
> On 2015 Jun 8, at 13:50, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Mon, Jun 8, 2015 at 1:45 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
>
> On 2015 Jun 8, at 13:16, Pete Cooper <peter_cooper at apple.com> wrote:
>
>
> On Jun 8, 2015, at 1:11 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Mon, Jun 8, 2015 at 12:19 PM, Pete Cooper <peter_cooper at apple.com>
> wrote:
> Hi all
>
> Recent work by Rafael and Duncan has greatly reduced the number of
> MCSymbol’s which are actually named.  In particular, temporary symbols
> typically don’t need names.  On the verify_uselist_order test case for
> example, only around 50,000 symbols have names out of 700,000 total symbols.
>
> This patch moves the storage for the Name* itself to be allocated prior to
> the MCSymbol.
>
> Any particular reason it goes before rather than after? (I know some of
> our data structures put things after)
>
> There’s a couple of subclasses of MCSymbol - MCSymbolCOFF and MCSymbolELF
> - so i thought it would be tricker to put it after those.
>
>
> There's no need to reinterpret_cast to MCSymbol when you just want a void*
> in operator new.
>
> Good catch.  Thanks.  I’ll fix that.
>
>
> FWIW, I've found the opposite in a few places in our code base.
> Sometimes you need to cast to `void *` to disambiguate from other
> versions of `operator new()`.
>
> Not quite sure I follow - In other contexts it may be relevant, but in
> that particular context (casting T* to U* just to return the U* as void*
> anyway) it doesn't seem to add anything.
>
>
> Sure, I didn't actually look at the code :).
>
>
>
>
> The alignment of the allocation might be interesting - given that it needs
> to be aligned as much as a pointer, not just MCSymbol (I assume MCSymbol is
> at least as aligned as a pointer right now, so you probably get this for
> free/correct without explicitly accounting for it)
>
> Thats true.  There are currently pointers in there, but good to be
> explicit about it.  I’ll fix that too.
>
>
> The extra indirection hidden behind NameTy is a bit confusing (especially
> since you have pointers to it - so it's pointers on pointers).
>
> Yeah, I see what you mean.  I’ll move the * out of NameTy.
>
> Thanks for the comments.  I’ll send out an updated patch shortly.
>
> Cheers,
> Pete
>
>
> This is similar to how User allocates Use’s in a custom new/delete.
>
> Given that a symbol is currently 48 bytes on MachO, this reduces it by 16%
> on the typical case.  I have other patches coming which will get us to
> 24-bytes which is on average half the current amount.
>
> Cheers,
> Pete
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150608/2b10f3e8/attachment.html>