Page MenuHomePhabricator

Pack symbols a bit more
ClosedPublic

Authored by espindola on Apr 25 2018, 1:56 PM.

Details

Reviewers
ruiu
grimar
Summary

Before this patch:

Symbol 56
Defined 80
Undefined 56
SharedSymbol 88
LazyArchive 72
LazyObject 56

With this patch

Symbol 48
Defined 72
Undefined 48
SharedSymbol 80
LazyArchive 64
LazyObject 48

The result is that peak allocation when linking chromium (according to heaptrack) goes from 578 to 568 MB.

Diff Detail

Event Timeline

espindola created this revision.Apr 25 2018, 1:56 PM
ruiu added a comment.Apr 25 2018, 2:00 PM

Maybe we should remove StringRefZ class and use a char pointer instead? The point of having StringRefZ class is that an instance of StringRefZ is automatically converted to StringRefZ when needed, but I think we no longer use the feature after this patch.

ELF/Symbols.h
53

uint32_t is probably better even though they are likely the same.

Maybe we should remove StringRefZ class and use a char pointer instead? The point of having StringRefZ class is that an instance of StringRefZ is automatically converted to StringRefZ when needed, but I think we no longer use the feature after this patch.

It is still convenient for the constructor argument. In some cases we know the size. Without it we have to overload the constructor so that there is a variant with "const char*" and one with StringRef.

espindola updated this revision to Diff 144010.Apr 25 2018, 2:33 PM

Use uint32_t

ruiu accepted this revision.Apr 25 2018, 2:41 PM

LGTM

It might be worth trying to search for a way to remove StringRefZ class cleanly, but this patch looks good as is. Thank you for doing this!

This revision is now accepted and ready to land.Apr 25 2018, 2:41 PM