Page MenuHomePhabricator

Place .text.hot section before .text.
AbandonedPublic

Authored by tcwang on Dec 13 2018, 3:04 PM.

Details

Summary

Objects with profiling information can mark symbols with .hot prefix/suffix.
With the option -z keep-text-section-prefix, all the symbols marked as hot
will be put into a separate section named as '.text.hot' (as well as cold
symbols, (.text.unlikely), .text.startup, .text.exit, .text.unliekly, etc.)

The previous version of lld does not specify ordering for these text sections
and all the sections with prefix comes after .text section. This doesn't benefit
performance in some cases and is different than the ordering of gold. (In gold,
these secctions are ordered as .text.hot, .text, .text.startup, .text.exit,
.text.unlikely)

This commit reorders the text sections by putting .text.hot before .text, in
the same order as gold (only for text sections with prefix, other section orders
are still different than gold as previous version).

The change only affects linking results when -z keep-text-section-prefix is passed.

Unit test text-section-prefix is changed accordingly.

Event Timeline

tcwang created this revision.Dec 13 2018, 3:04 PM
pcc added a subscriber: pcc.Dec 13 2018, 3:13 PM

I don't understand why this ordering should always result in better performance. To the extent that it affects performance at all (e.g. on ARM a particular order might allow the linker to create fewer range extension thunks) it would seem to be arbitrary which order would be better for any given program.

Chrome OS developed a dependency (perf, not functional) on things in .text.hot coming before things in .text. Namely, they have code that maps the first N MB of Chrome's .text section to hugepages, under the assumption that the code near the beginning of the binary will be hotter than the code near the end of the binary.

Naturally, this says nothing about how general programs will perform with this change. I similarly expect this to be more or less a wash for many programs.

I don't frequent lld often, but my 2c are that emulating this in lld is trivial and doing so AFAICT loses us nothing. Hence, I don't see a compelling reason to avoid this change. Since a user's tripped over this to the tune of a few percent perf regression (?), I'm more in favor than not.

Like said, I don't poke lld very much, so I have no plans to be the one who stamps this :)

Care to update the title of this review? ;)

tcwang retitled this revision from # Enter a commit message. # to Place .text.hot section before .text..Dec 13 2018, 4:31 PM
tcwang edited the summary of this revision. (Show Details)
tcwang removed a reviewer: gbiv.

Care to update the title of this review? ;)

Just did. Thanks!

pcc added a comment.Dec 13 2018, 4:56 PM

Thanks for the explanation.

It may seem like a trivial change now, but changes like this add up and may result in increased complexity later (e.g. suppose that we wanted to add an --output-section-ordering-file feature; this would need to interact somehow with this change), and once you start emulating another linker like this it can be difficult to undo the change since you don't know who might be unintentionally depending on the emulation. Unless there's evidence that more programs are relying on this assumption I think it's premature to code it into the linker.

I also consider it a bug for ChromeOS to make this assumption. This isn't the first lld transition issue we've seen whose root cause turned out to be an incorrect assumption. Can't you change ChromeOS so that instead of assuming that the first few pages are hot it just moves the pages covered by .text.hot into hugepages? It's possible to use a linker script to get the address of the start and end of .text.hot. See for example:
https://cs.chromium.org/chromium/src/base/android/library_loader/anchor_functions.lds
and how the symbols defined by that file are used.

Assuming that this is the code that implements hugepages:
https://cs.chromium.org/chromium/src/chromeos/hugepage_text/hugepage_text.cc
it would seem to result in simplifying the code a little as well.

grimar added a subscriber: grimar.Dec 14 2018, 12:38 AM
In D55679#1330672, @pcc wrote:

Thanks for the explanation.

It may seem like a trivial change now, but changes like this add up and may result in increased complexity later (e.g. suppose that we wanted to add an --output-section-ordering-file feature; this would need to interact somehow with this change), and once you start emulating another linker like this it can be difficult to undo the change since you don't know who might be unintentionally depending on the emulation. Unless there's evidence that more programs are relying on this assumption I think it's premature to code it into the linker.

I also consider it a bug for ChromeOS to make this assumption. This isn't the first lld transition issue we've seen whose root cause turned out to be an incorrect assumption. Can't you change ChromeOS so that instead of assuming that the first few pages are hot it just moves the pages covered by .text.hot into hugepages? It's possible to use a linker script to get the address of the start and end of .text.hot. See for example:
https://cs.chromium.org/chromium/src/base/android/library_loader/anchor_functions.lds
and how the symbols defined by that file are used.

Assuming that this is the code that implements hugepages:
https://cs.chromium.org/chromium/src/chromeos/hugepage_text/hugepage_text.cc
it would seem to result in simplifying the code a little as well.

We agree that this is a bug for ChromeOS to make assumption based on gold. We will seek solution to fix it in the long term. I guess we might just keep the patch as a temporary workaround for now. We'll discard the patch and close it.

Thanks everyone for putting time to review it!

tcwang abandoned this revision.Dec 14 2018, 1:51 PM