Reorder symbols when linking libxul

RESOLVED FIXED in mozilla13

Status

()

Core
Build Config
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 9 obsolete attachments)

11.87 KB, patch
glandium
: review+
Details | Diff | Splinter Review
35.57 KB, patch
khuey
: review+
Details | Diff | Splinter Review
15.55 KB, patch
khuey
: review+
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
Filing this bug to track the progress on this subject.

I made an attempt today reordering object files with profiling data coming from a modified icegrind. The test was done on linux x86-64, after the application of the patches from bug 569629. Bug 569629 itself saves about 200ms off around a 4 seconds startup, and .o reordering further saved 500ms!

I'll try to integrate with fakelibs and push to try to see the effect on windows and mac, which is expected to be quite positive.
(Assignee)

Comment 1

7 years ago
Created attachment 483995 [details] [diff] [review]
WIP patch

This is a first implementation depending on the patches I sent to bug 584474, bug 605146 and bug 605153.
The order file used was generated from profiling on linux x86-64 only. I'm waiting for the try builds to see if that has some positive effect on mac and windows, already.
Assignee: nobody → mh+mozilla
(Assignee)

Comment 2

7 years ago
Try server builds will be available here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mh@glandium.org-c6eebd7373de/

(some are already)

Surprisingly, the linux64 ts_cold result doesn't exhibit the improvement I'm seeing locally, though the binary is correctly reordered.
(Assignee)

Updated

7 years ago
Depends on: 605406
(Assignee)

Comment 3

7 years ago
I can't get reliable numbers on mac, but it looks like it is definitely a win. ts_cold on mac64 seems to agree, too. http://perf.snarkfest.net/compare-talos/?oldRevs=e68dd8c5d9cd&newRev=c6eebd7373de&tests=ts_cold&submit=true

On Windows, LTO is getting in the way, and no change can be seen. Though looking at the symbols ordering, I'm wondering what exactly LTO is trying to do... I'd be interested in icegrind-like results on windows.
(Assignee)

Comment 4

7 years ago
Created attachment 493646 [details] [diff] [review]
WIP patch

New version against new patches from bug 584474. The reorder list still needs to be tweaked, though.
Attachment #483995 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Depends on: 616262
(Assignee)

Comment 5

7 years ago
Created attachment 495889 [details] [diff] [review]
WIP patch v3

Same as the previous one, but with a better profile after bug 616262.
Attachment #493646 - Attachment is obsolete: true

Comment 6

7 years ago
It feels a bit fragile to me to use a static list of files here, could that somehow be dynamically generated instead?
(Assignee)

Comment 7

7 years ago
(In reply to comment #6)
> It feels a bit fragile to me to use a static list of files here, could that
> somehow be dynamically generated instead?

There are two problems with a dynamically generated list:
- it requires tools that aren't currently in the mozilla build toolbox
- it actually requires manual tweaking to really be awesome.

Seeing how a month old list is still pretty efficient at improving startup time (cf. http://glandium.org/blog/?p=1296 ). Actually the current list would only require a few tweaks for new static initializers and a few new object files.

An advantage of having a static list is that distributors don't need to dynamically generate the list, which is a heavyweight process, to get the gains from this reordering.
If we're going for a manually generated list, then I think we need comprehensive instructions on how to build that list - pointed to from the file itself and probably stored on devmo or something.

It may also need to become part of run-up-to-final release checklists that the tweaking has been done.
I would assume that an out-of-date manually generated list is not really worse than no list at all, in which case you get the quasi-random ordering we've always had.
(Assignee)

Comment 10

7 years ago
Created attachment 504488 [details] [diff] [review]
part 1 - Add an option to expandlibs-exec to allow to reorder the objects list

Updated to fit last changes in bug 584474. I'm now separating the implementation and the actual reordering list.
Attachment #495889 - Attachment is obsolete: true
Attachment #504488 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 11

7 years ago
Created attachment 504490 [details] [diff] [review]
part 2 - Use an object reorder list for libxul

This is the same list as the previous one, I'll update with a fresh profile soon.
Comment on attachment 504488 [details] [diff] [review]
part 1 - Add an option to expandlibs-exec to allow to reorder the objects list

>diff --git a/config/config.mk b/config/config.mk
>--- a/config/config.mk
>+++ b/config/config.mk
>@@ -842,10 +842,10 @@ endif
> OPTIMIZE_JARS_CMD = $(PYTHON) $(call core_abspath,$(topsrcdir)/config/optimizejars.py)
> 
> EXPAND_LIBS = $(PYTHON) $(DEPTH)/config/expandlibs.py
> EXPAND_LIBS_EXEC = $(PYTHON) $(topsrcdir)/config/pythonpath.py -I$(DEPTH)/config $(topsrcdir)/config/expandlibs-exec.py
> EXPAND_LIBS_GEN = $(PYTHON) $(topsrcdir)/config/pythonpath.py -I$(DEPTH)/config $(topsrcdir)/config/expandlibs-gen.py
> EXPAND_AR = $(EXPAND_LIBS_EXEC) --extract -- $(AR)
> EXPAND_CC = $(EXPAND_LIBS_EXEC) --uselist -- $(CC)
> EXPAND_CCC = $(EXPAND_LIBS_EXEC) --uselist -- $(CCC)
>-EXPAND_LD = $(EXPAND_LIBS_EXEC) --uselist -- $(LD)
>-EXPAND_MKSHLIB = $(EXPAND_LIBS_EXEC) --uselist -- $(MKSHLIB)
>+EXPAND_LD = $(EXPAND_LIBS_EXEC) --uselist $(if $(REORDER),--reorder $(REORDER) --extract )-- $(LD)
>+EXPAND_MKSHLIB = $(EXPAND_LIBS_EXEC) --uselist $(if $(REORDER),--reorder $(REORDER) --extract )-- $(MKSHLIB)

If you always need --extract to reorder, why not just make --reorder imply --extract?

>diff --git a/config/expandlibs-exec.py b/config/expandlibs-exec.py
>--- a/config/expandlibs-exec.py
>+++ b/config/expandlibs-exec.py
>+    def reorder(self, order_file):
>+        '''Given a list of file names without OBJ_SUFFIX, rearrange self
>+        so that the object file names it contains follow that list.
>+        '''
>+        order = map(lambda s: s.strip(), open(order_file).readlines())

order = [line.strip() for line in open(order_file).readlines()]

or if you don't expect trailing whitespace, just newlines, you can just do:

order = open(order_file).read().splitlines()

>+        objs = filter(lambda item: os.path.splitext(item)[1] == OBJ_SUFFIX, self)

objs = [item for item in self if os.path.splitext(item)[1] == OBJ_SUFFIX]

>+        if not len(objs): return

if not objs: works here, empty lists evaluate as False.

>+        idx = self.index(objs[0])
>+        objnames = dict(map(lambda item: [os.path.splitext(os.path.basename(item))[0],item], objs))
>+        ordered = map(lambda item: objnames[item], filter(lambda item: item in objnames.keys(), order))
>+        ordered += filter(lambda item: item not in ordered, objs)
>+        self[0:] = self[0:idx] + ordered + filter(lambda item: os.path.splitext(item)[1] != OBJ_SUFFIX, self[idx:])
>+

This seems...unnecssarily complex. Here's how I'd write this logic:

order = [[line.strip() + OBJ_SUFFIX for line in open(order_file).readlines()]
if any(o not in self for o in order):
  error
objs = [o for o in self if o.endswith(OBJ_SUFFIX)]
idx = self.index(objs[0])
# keep everything before the first object, then the ordered objects, then any other objects, then any non-objects after the first object
self[0:] = self[0:idx] + order + [o for o in objs if o not in order] + [x for x in self[idx:] if not x.endswith(OBJ_SUFFIX)]

Other than that it looks good.
Attachment #504488 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 13

6 years ago
Created attachment 514767 [details] [diff] [review]
part 1 - Add an option to expandlibs-exec to allow to reorder the objects list, v2

This addresses review from comment 12 and adds a testcase.
Attachment #504488 - Attachment is obsolete: true
Attachment #514767 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 14

6 years ago
Created attachment 514815 [details] [diff] [review]
part 1 - Add an option to expandlibs-exec to allow to reorder the objects list, v2.1

There were missing parens.
Attachment #514767 - Attachment is obsolete: true
Attachment #514815 - Flags: review?(ted.mielczarek)
Attachment #514767 - Flags: review?(ted.mielczarek)
Comment on attachment 514815 [details] [diff] [review]
part 1 - Add an option to expandlibs-exec to allow to reorder the objects list, v2.1

>diff --git a/config/expandlibs_exec.py b/config/expandlibs_exec.py
>--- a/config/expandlibs_exec.py
>+++ b/config/expandlibs_exec.py
>+    def reorder(self, order_list):
>+        '''Given a list of file names without OBJ_SUFFIX, rearrange self
>+        so that the object file names it contains are ordered according to
>+        that list.
>+        '''
>+        objs = [o for o in self if o.endswith(conf.OBJ_SUFFIX)]
>+        if not objs: return

return goes on a new line.

Looks fine otherwise.
Attachment #514815 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Updated

6 years ago
Attachment #514815 - Attachment filename: diff → bug603370-1
(Assignee)

Comment 16

6 years ago
Created attachment 588912 [details] [diff] [review]
part 1 - Add an option to expandlibs-exec to allow to reorder the objects list.

Refreshed against current trunk.
(Assignee)

Updated

6 years ago
Attachment #514815 - Attachment is obsolete: true
(Assignee)

Comment 17

6 years ago
Comment on attachment 588912 [details] [diff] [review]
part 1 - Add an option to expandlibs-exec to allow to reorder the objects list.

Carrying over r+
Attachment #588912 - Flags: review+
(Assignee)

Comment 18

6 years ago
Landed part 1
https://hg.mozilla.org/integration/mozilla-inbound/rev/283408b8d8a3
https://hg.mozilla.org/mozilla-central/rev/283408b8d8a3

Leaving open for part 2.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla12
(Assignee)

Comment 20

5 years ago
It turns out reordering objects doesn't have so much benefit. Reordering symbols does.
Status: ASSIGNED → NEW
Summary: Reorder object files when linking libxul → Reorder symbols when linking libxul
Target Milestone: mozilla12 → ---
Version: Trunk → Other Branch
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Version: Other Branch → Trunk
(Assignee)

Comment 21

5 years ago
Created attachment 601632 [details] [diff] [review]
Add an option to expandlibs-exec to allow to reorder the symbols when linking

This applies on top of a backout of part 1 (changeset 283408b8d8a3). It only works for ELF linking. Object reordering didn't really work on other targets anyways.

This new version takes a list of symbols, resolves them into sections, and uses the list of sections to feed the linker. Both GNU ld and gold are supported.
Attachment #601632 - Flags: review?(khuey)
(Assignee)

Updated

5 years ago
Attachment #504490 - Attachment is obsolete: true
(Assignee)

Comment 22

5 years ago
Created attachment 602854 [details] [diff] [review]
Add an option to expandlibs-exec to allow to reorder the symbols when linking

Reordering with gold was not working entirely. (it was indeed reordering, but putting reordered stuff at the end, not at the beginning)
Attachment #602854 - Flags: review?(khuey)
(Assignee)

Updated

5 years ago
Attachment #601632 - Attachment is obsolete: true
Attachment #601632 - Flags: review?(khuey)
(Assignee)

Comment 23

5 years ago
Created attachment 603240 [details] [diff] [review]
Avoid Identical Code Folding messing with symbol reordering
Attachment #603240 - Flags: review?(khuey)
(Assignee)

Comment 24

5 years ago
Created attachment 603241 [details] [diff] [review]
Avoid Identical Code Folding messing with symbol reordering

Small testcase change.
Attachment #603241 - Flags: review?(khuey)
(Assignee)

Updated

5 years ago
Attachment #603240 - Attachment is obsolete: true
Attachment #603240 - Flags: review?(khuey)
Comment on attachment 602854 [details] [diff] [review]
Add an option to expandlibs-exec to allow to reorder the symbols when linking

Review of attachment 602854 [details] [diff] [review]:
-----------------------------------------------------------------

A few nits to pick.

::: build/autoconf/expandlibs.m4
@@ +30,5 @@
> +
> +LIBS_DESC_SUFFIX=desc
> +AC_SUBST(LIBS_DESC_SUFFIX)
> +AC_SUBST(EXPAND_LIBS_LIST_STYLE)
> +if test "$GCC_USE_GNU_LD"; then

Put a newline between these.

@@ +41,5 @@
> +             EXPAND_LIBS_ORDER_STYLE=section-ordering-file,
> +             EXPAND_LIBS_ORDER_STYLE=)
> +         LDFLAGS="$_SAVE_LDFLAGS"
> +         if test -z "$EXPAND_LIBS_ORDER_STYLE"; then
> +             if AC_TRY_COMMAND(${CC-cc} ${DSO_LDOPTS} ${LDFLAGS} -o conftest${DLL_SUFFIX} -Wl,--verbose 2> /dev/null | sed -n '/^===/,/^===/p' | grep '\.text'); then

$(DLL_PREFIX)conftest$(DLL_SUFFIX)

@@ +46,5 @@
> +                 EXPAND_LIBS_ORDER_STYLE=linkerscript
> +             else
> +                 EXPAND_LIBS_ORDER_STYLE=none
> +             fi
> +             rm -f conftest${DLL_SUFFIX}

Same.

::: config/config.mk
@@ +789,5 @@
>  EXPAND_AR = $(EXPAND_LIBS_EXEC) --extract -- $(AR)
>  EXPAND_CC = $(EXPAND_LIBS_EXEC) --uselist -- $(CC)
>  EXPAND_CCC = $(EXPAND_LIBS_EXEC) --uselist -- $(CCC)
>  EXPAND_LD = $(EXPAND_LIBS_EXEC) --uselist -- $(LD)
> +EXPAND_MKSHLIB = $(EXPAND_LIBS_EXEC) --uselist $(if $(SYMBOL_ORDER),--symbol-order $(SYMBOL_ORDER) )-- $(MKSHLIB)

I'd prefer
if $(SYMBOL_ORDER)
SYMBOL_ORDER_ARGS = --symbol-order $(SYMBOL_ORDER)
fi
EXPAND_MKSHLIB = $(EXPAND_LIBS_EXEC) --uselist $(SYMBOL_ORDER_ARGS) -- $(MKSHLIB)

o rsomething similar.
Attachment #602854 - Flags: review?(khuey) → review+
Attachment #603241 - Flags: review?(khuey) → review+
(Assignee)

Comment 26

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b054a6df1b52
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9d268575baa
https://hg.mozilla.org/integration/mozilla-inbound/rev/42ddb716c71d
(Assignee)

Comment 27

5 years ago
Fixup:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59148c7434aa
https://hg.mozilla.org/mozilla-central/rev/b054a6df1b52
https://hg.mozilla.org/mozilla-central/rev/c9d268575baa
https://hg.mozilla.org/mozilla-central/rev/42ddb716c71d
https://hg.mozilla.org/mozilla-central/rev/59148c7434aa
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Depends on: 746650

Updated

3 years ago
See Also: → bug 974308
You need to log in before you can comment on or make changes to this bug.