Reorder symbols when linking libxul

RESOLVED FIXED in mozilla13

Status

defect
RESOLVED FIXED
9 years ago
Last year

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
mozilla13
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

9 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

9 years ago
Posted patch WIP patch (obsolete) — Splinter Review
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

9 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

9 years ago
Depends on: 605406
Assignee

Comment 3

9 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

9 years ago
Posted patch WIP patch (obsolete) — Splinter Review
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

9 years ago
Depends on: 616262
Assignee

Comment 5

9 years ago
Posted patch WIP patch v3 (obsolete) — Splinter Review
Same as the previous one, but with a better profile after bug 616262.
Attachment #493646 - Attachment is obsolete: true

Comment 6

9 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

9 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

9 years ago
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

9 years ago
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

8 years ago
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

8 years ago
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

8 years ago
Attachment #514815 - Attachment filename: diff → bug603370-1
Assignee

Updated

8 years ago
Attachment #514815 - Attachment is obsolete: true
Assignee

Comment 17

8 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+
https://hg.mozilla.org/mozilla-central/rev/283408b8d8a3

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

Comment 20

7 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

7 years ago
Status: NEW → ASSIGNED
Version: Other Branch → Trunk
Assignee

Comment 21

7 years ago
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

7 years ago
Attachment #504490 - Attachment is obsolete: true
Assignee

Comment 22

7 years ago
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

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

Comment 24

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

Updated

7 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+

Updated

5 years ago
See Also: → 974308

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.