1523352 - Track libxul export table size in perfherder

Status: RESOLVED INACTIVE

(Testing :: Talos, enhancement, P3)

Description

[(Away)]

I believe this could be done with a relatively straightforward modification of buildbase.py [1] with support from rust-size via goblin [2].

[1] https://searchfox.org/mozilla-central/rev/4faab2f1b697827b93e16cb798b22b197e5235c9/testing/mozharness/mozharness/mozilla/building/buildbase.py#1549
[2] https://docs.rs/goblin/0.0.19/goblin/pe/data_directories/struct.DataDirectories.html#method.get_export_table

Motivation:

When we import third-party libraries into xul.dll, they usually have code that goes like:

#ifdef _WIN32
#define MYLIB_EXPORT __declspec(dllexport)
#else
#define MYLIB_EXPORT attribute((visibility("default")))
#endif

MYLIB_EXPORT int MyLibrary_DoTheThing() { return 42; }

This makes perfect sense when upstream builds their product as MyLibrary.dll, as those APIs need to be callable from the outside world. But when we paste the code into libxul, and its callers are also in libxul, there is no need to cross a library boundary.

Exporting those symbols when not needed is harmful for several reasons:

• It prevents optimization and/or increases code size. The compiler has to choose between keeping a single copy of the function with the shape demanded by the ABI, which inhibits interprocedural optimizations; or creating optimized/inlined copies but keeping the original just in case the outside world ever calls it.

• It attracts wrongful blame in crash stacks. In cases where a debugger doesn't have access to symbols, it will guess an instruction's function based on the nearest exported symbol. This often makes it look like function_that_landed_last_week+0x12345 caused a crash, which wastes time with a wild goose chase.

• Exported symbols invite function hooks from questionable software.

Comment 1

[Eric Rahm [:erahm]]

I can take a look at the rust-size piece, ni? to remind myself.

Comment 2

[Eric Rahm [:erahm]]

I have a PR up for review on the rust-size side of things.

Comment 3

[Eric Rahm [:erahm]]

This is available in rust-size as the export_table entry in the data section for PE and Mach files as of fa16628. For PE it's the size of the export table and for Mach it's the number of entries. For Elf there should already be the dynsym entry.

Comment 4

[(Away)]

Now that the rust-size part of this is done, I had figured this would be as simple as adding export_table to section_interests here: https://searchfox.org/mozilla-central/rev/6c9f60f8cc064a1005cd8141ecd526578ae9da7a/testing/mozharness/mozharness/mozilla/building/buildbase.py#1498

But, poking around at how this data is reported, that would make export_table appear as a subtest, alongside others such as https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=mozilla-central&originalRevision=267ddc3595fe2147c72e97e97cdd25f3a7519fa9&newRevision=9617ee5d9b886a899730077b207655222d27867a&originalSignature=1688161&newSignature=1688161&framework=2

On my nightly, export_table measures at 404, so even if it grew by orders of magnitude, the change would get completely washed out by the other subtests whose values are in the millions, and we'd never notice.

jmaher, is my understanding correct? If so, is there any way to make this subtest more sensitive to changes? Or is the only solution to make it not be a subtest?

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

redirect to davehunt- he is in charge of perf these days- in general the subtests are summarized via geometric_mean, so values do not need to be so extreme to be picked up. With that said, there is a large range. If this is made to be a top level metric, then it needs to be documented properly and get agreement from the perf sheriffs (again :davehunt)

Comment 6

[(Away)]

in general the subtests are summarized via geometric_mean

I had thought that too, but the value reported for "XUL section sizes" for https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=9617ee5d9b886a899730077b207655222d27867a&searchStr=win%2Copt%2Cbuild%2C64&selectedJob=247721878, as well as the associated graph, is just the sum of the subtests. Am I looking at the wrong thing?

Comment 7

[Eric Rahm [:erahm]]

(In reply to David Major [:dmajor] from comment #6)

in general the subtests are summarized via geometric_mean

I had thought that too, but the value reported for "XUL section sizes" for https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=9617ee5d9b886a899730077b207655222d27867a&searchStr=win%2Copt%2Cbuild%2C64&selectedJob=247721878, as well as the associated graph, is just the sum of the subtests. Am I looking at the wrong thing?

It's customary to use geomean, but neither required nor automatic. I think it's okay to make this a "top-level" test, it'll still be nested under build_metrics.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

The build metrics Talos tests are documented at https://wiki.mozilla.org/Performance_sheriffing/Talos/Tests#Build_Metrics but this appears to be missing information. I don't have any objections to adding this as either test or subtest, although a new test will have more impact on the perf sheriffs. Whichever we decide, can we please update the linked wiki page with details such as the test contact? You can see examples on that page of how other tests are documented. In the near future we plan to enforce a minimum level of documentation for all perf tests.

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

:dmajor please update the wiki docs at https://wiki.mozilla.org/Performance_sheriffing/Talos/Tests once this is resolved.

Comment 10

[Greg Mierzwinski [:sparky]]

Closing as inactive.