[PATCH] Allocate MCSymbol Name intrusively only when required

[Pete Cooper]

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/231b7463/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mcsymbol-name.diff
Type: application/octet-stream
Size: 5757 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150608/231b7463/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150608/231b7463/attachment-0001.html>