1441051 - Remove the need to run `mach buildsymbols` after `mach build [binary]` in order to get profiles with symbols for local Windows builds

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

I have a mozconfig specifically for profiling (with opt enabled, debug disabled, and rust opt level set to 2). Sometimes I need to add code or apply patch then remeasure. Running buildsymbols is an extra step I need to do every time, which I sometimes forgot and leading to a useless profile.

It would be nice if I can put an option into mozconfig to let it generate symbols automatically after build, that way I don't need to remember that.

Also, usually I only change code in xul.dll, but buildsymbols seems to always rebuild symbols for everything, which looks wasteful as well. Probably it can work like "make" to just regenerate symbols for changed binaries.

Comment 1

[(not currently active) Ted Mielczarek]

We actually have exactly this functionality in the build system, but it's conditioned on MOZ_AUTOMATION and MOZ_AUTOMATION_BUILD_SYMBOLS right now:
https://dxr.mozilla.org/mozilla-central/rev/7208b6a7b11c3ed8c87a7f17c9c30a8f9583e791/config/rules.mk#813

We do this so that symbol dumping happens as soon as possible during the build, to avoid it being a long pole at the end.

We could probably move those tests into configure, and add a variable that you could explicitly set in your mozconfig to control this behavior.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

That would be good, and it would be even better if it can be optimized for incremental build (e.g. symbols of test exes usually don't need to get updated), but probably xul.dll is always the longest one so it may not matter that much...

Comment 3

[Mike Hommey [:glandium]]

Why do you need to run buildsymbols? I don't think that's necessary on mac and linux, why is it necessary on windows?

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

To make Gecko Profiler useful on a local build.

Comment 5

[Mike Hommey [:glandium]]

Let me rephrase: why does the profiler need you to run buildsymbols on windows, when, afaik, it doesn't on other platforms?

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

That's something beyond my knowledge. The profile doesn't show any symbol if you don't do that on Windows, and this step is documented on MDN: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Profiling_with_the_Built-in_Profiler_and_Local_Symbols_on_Windows#Profiling_local_builds_(without_using_talos)

Comment 7

[(not currently active) Ted Mielczarek]

I think the profiler can use `addr2line` on Linux/Mac, but it doesn't have any code for getting symbols from PDB files on Windows, so it reads .sym files. Ideally it'd be able to read .pdb files using either Microsoft's APIs or some other code.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> I think the profiler can use `addr2line` on Linux/Mac, but it doesn't have
> any code for getting symbols from PDB files on Windows, so it reads .sym
> files. Ideally it'd be able to read .pdb files using either Microsoft's APIs
> or some other code.

Sounds like we should compile some Rust .pdb reader to wasm! :p

Comment 9

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> I think the profiler can use `addr2line` on Linux/Mac

It doesn't anymore. IIRC it now uses rust code compiled to wasm. Or something along these lines. So the question becomes, why it's not reading pdbs the same way?

Comment 10

[Markus Stange [:mstange]]

(In reply to Mike Hommey [:glandium] from comment #9)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> > I think the profiler can use `addr2line` on Linux/Mac
> 
> It doesn't anymore. IIRC it now uses rust code compiled to wasm.

Not anymore. Now it uses nm on Mac / Linux: https://searchfox.org/mozilla-central/rev/056a4057575029f58eb69ae89bc7b6c3c11e2e28/browser/components/extensions/ext-geckoProfiler.js#115-141

> So the question becomes, why it's not reading pdbs the same way?

When we were still using dump-syms compiled to asm.js, we only did it for Linux + Mac because the Windows version of dump-syms used Windows headers and could not compile to JS.

I'd love to have something that's written in rust and can read pdbs, and that we can compile to WASM.

Comment 11

[(not currently active) Ted Mielczarek]

There is a Rust PDB crate (I've contributed some code to it recently) but I don't know if it has enough things implemented to look up functions by address like we'd need to do here: https://github.com/willglynn/pdb

I'd love to get it to that point, though, as I'd love to use that functionality in other places!

Comment 12

[Markus Stange [:mstange]]

That crate has enough functionality to list all symbols with their addresses, which is what I need. But I think I also need something that knows how to demangle MSVC symbols, e.g ?GetInputDataSourceSurface@FilterNodeSoftware@gfx@mozilla@@IAE?AU?$already_AddRefed@VDataSourceSurface@gfx@mozilla@@@@IABU?$IntRectTyped@UUnknownUnits@gfx@mozilla@@@23@W4FormatHint@123@W4ConvolveMatrixEdgeMode@23@PBU523@@Z

Comment 13

[(not currently active) Ted Mielczarek]

I don't know of any Rust code for that currently. DbgHelp.dll does have an API for demangling:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms681400(v=vs.85).aspx

and there's a commandline undname.exe that ships with MSVC:
https://msdn.microsoft.com/en-us/library/5x49w699.aspx

Wine has an LGPL C implementation of it:
https://github.com/wine-mirror/wine/blob/master/dlls/msvcrt/undname.c

Comment 14

[Markus Stange [:mstange]]

Jeff found an MIT-licensed C++ version at https://github.com/rui314/msvc-demangler . I might take a stab at converting it to rust.

Comment 15

[Mike Hommey [:glandium]]

(In reply to Markus Stange [:mstange] from comment #14)
> Jeff found an MIT-licensed C++ version at
> https://github.com/rui314/msvc-demangler . I might take a stab at converting
> it to rust.

there is also https://github.com/nico/demumble

Comment 16

[Markus Stange [:mstange]]

> https://github.com/nico/demumble

That one uses Wine's LGPL code.

Comment 17

[Markus Stange [:mstange]]

Unfortunately, it turns out that https://github.com/rui314/msvc-demangler is rather incomplete.

As a very pragmatic workaround, I propose the following: If we are running in a local build, then dump_syms.exe exists in the object directory. We can make the geckoProfiler API run dump_syms manually on any pdb files that it's asked to symbolicate.
It might not be as fast as a WASM solution that only grabs the subset of information that we actually need, but it means that we don't have to ship a big WASM blob for a feature that's only used by an extremely small subset of our users.

Comment 18

[Markus Stange [:mstange]]

Attachment: Bug 1441051 - Automatically invoke dump_syms.exe on Windows when the profiler asks for symbol tables.

Review commit: https://reviewboard.mozilla.org/r/226966/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226966/

Comment 19

[Markus Stange [:mstange]]

Xidorn, could you try this patch out?

Comment 20

[Markus Stange [:mstange]]

Comment on attachment 8958026 [details]
Bug 1441051 - Automatically invoke dump_syms.exe on Windows when the profiler asks for symbol tables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226966/diff/1-2/

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Yeah, it works for me. Thanks.

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 8958026 [details]
Bug 1441051 - Automatically invoke dump_syms.exe on Windows when the profiler asks for symbol tables.

https://reviewboard.mozilla.org/r/226966/#review232858

r- only because I think you should cache the output of dump_syms or people are going to get annoyed with how long it takes to run, but otherwise this seems like a sensible approach.

It would be nice if we could sort out the PDB reading stuff in the future, but having something that works is more useful than having a great solution.

::: browser/components/extensions/ext-geckoProfiler.js:179
(Diff revision 2)
> +  // environment variable in order to increase the chances of dump_syms
> +  // succeeding.
> +  const programFilesX86Path = env["PROGRAMFILES(X86)"] || env["PROGRAMFILES"];
> +  if (programFilesX86Path) {
> +    // Check if we have vswhere. This is a utilty that gets installed into a
> +    // fixed path and can be used to look up the actual location of the Visual

n.b., if it's helpful, there's also a copy of vswhere checked into the tree under build/win32.

::: browser/components/extensions/ext-geckoProfiler.js:220
(Diff revision 2)
> +  env.PATH = existingPaths.concat(extraPaths).join(";");
> +  const opts = {
> +    command: dumpSymsPath,
> +    arguments: [debugPath],
> +    environment: env,
> +    stderr: "stdout" // dump_syms.exe writes its output to stderr

Mixing stderr and stdout probably isn't what you want here.

::: browser/components/extensions/ext-geckoProfiler.js:225
(Diff revision 2)
> +    stderr: "stdout" // dump_syms.exe writes its output to stderr
> +  };
> +  const proc = await Subprocess.call(opts);
> +  const chunks = [];
> +  await readAllData(proc.stdout, data => chunks.push(data));
> +  const textBuffer = joinBuffers(chunks);

dupm_syms takes forever on xul.pdb. It's probably worthwhile for this code to cache the output. Just writing xul.sym next to the xul.pdb file should be fine, although you'd want to compare the modification times of the files then.

I don't know what usage patterns of developers are, honestly. If they rebuild then obviously the symbols change so you'd need to re-run dump_syms. If they just restart Firefox without rebuilding then you should be able to use the same symbols.

Comment 23

[Markus Stange [:mstange]]

The symbol tables which we return from the WebExtensions API are cached by perf.html. perf.html will only ask for a given symbol table once.

Comment 24

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8958026 [details]
Bug 1441051 - Automatically invoke dump_syms.exe on Windows when the profiler asks for symbol tables.

https://reviewboard.mozilla.org/r/226966/#review232886

Comment 25

[(not currently active) Ted Mielczarek]

Comment on attachment 8958026 [details]
Bug 1441051 - Automatically invoke dump_syms.exe on Windows when the profiler asks for symbol tables.

https://reviewboard.mozilla.org/r/226966/#review233204

In light of mstange enlightening me to how perf-html caches symbols I am revising this to an r+.

Comment 26

[Markus Stange [:mstange]]

Comment on attachment 8958026 [details]
Bug 1441051 - Automatically invoke dump_syms.exe on Windows when the profiler asks for symbol tables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226966/diff/2-3/

Comment 27

[Markus Stange [:mstange]]

Comment on attachment 8958026 [details]
Bug 1441051 - Automatically invoke dump_syms.exe on Windows when the profiler asks for symbol tables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226966/diff/3-4/

Comment 28

[Code Review Bot [:reviewbot]]

Comment on attachment 8958026 [details]
Bug 1441051 - Automatically invoke dump_syms.exe on Windows when the profiler asks for symbol tables.

https://reviewboard.mozilla.org/r/226966/#review236400


Code analysis found 7 defects in this patch:
 - 7 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/extensions/ext-geckoProfiler.js:92
(Diff revision 3)
>      });
>      return promise;
>    }
>  }
>  
> +const joinBuffers = function (buffers) {

Error: Unexpected space before function parentheses. [eslint: space-before-function-paren]

::: browser/components/extensions/ext-geckoProfiler.js:174
(Diff revision 3)
> +    if (varname.toLowerCase() == name.toLowerCase()) {
> +      return value;
> +    }
> +  }
> +  return undefined;
> +}

Error: Missing semicolon. [eslint: semi]

::: browser/components/extensions/ext-geckoProfiler.js:193
(Diff revision 3)
> +  if (programFilesX86Path) {
> +    // Check if we have vswhere. This is a utilty that gets installed into a
> +    // fixed path and can be used to look up the actual location of the Visual
> +    // Studio installation. It's available starting with VS2017.
> +    const vswherePath = OS.Path.join(programFilesX86Path,
> +      "Microsoft Visual Studio", "Installer", "vswhere.exe");

Error: Expected indentation of 37 spaces but found 6. [eslint: indent]

::: browser/components/extensions/ext-geckoProfiler.js:199
(Diff revision 3)
> +    const args = [
> +      "-products", "*",
> +      "-latest",
> +      "-requires", "Microsoft.VisualStudio.Component.VC.Tools.x86.x64",
> +      "-property", "installationPath",
> +      "-format", "value"

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/ext-geckoProfiler.js:208
(Diff revision 3)
> +        (await runCommandAndGetOutputAsString(vswherePath, args)).trim();
> +      if (vsInstallationPath) {
> +        const diaSDKPath = OS.Path.join(vsInstallationPath, "DIA SDK");
> +        return [
> +          OS.Path.join(diaSDKPath, "bin"),
> +          OS.Path.join(diaSDKPath, "bin", "amd64")

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/ext-geckoProfiler.js:220
(Diff revision 3)
> +    }
> +  }
> +  // Add no extra DLL search paths and hope that dump_syms.exe is going to
> +  // succeed regardless.
> +  return [];
> +}

Error: Missing semicolon. [eslint: semi]

::: browser/components/extensions/ext-geckoProfiler.js:231
(Diff revision 3)
> +  env.PATH = existingPaths.concat(extraPaths).join(";");
> +  const opts = {
> +    command: dumpSymsPath,
> +    arguments: [debugPath],
> +    environment: env,
> +    stderr: "pipe"

Error: Missing trailing comma. [eslint: comma-dangle]

Comment 29

[Markus Stange [:mstange]]

Comment on attachment 8958026 [details]
Bug 1441051 - Automatically invoke dump_syms.exe on Windows when the profiler asks for symbol tables.

https://reviewboard.mozilla.org/r/226966/#review232858

> Mixing stderr and stdout probably isn't what you want here.

You're right. I only did this because I didn't receive any data in the stdout pipe otherwise. I've changed it to stderr: "pipe" and am now correctly receiving the output in the stdout pipe. This might be a bug in the Windows implementation of Subprocess.jsm.

Comment 30

[Markus Stange [:mstange]]

Comment on attachment 8958026 [details]
Bug 1441051 - Automatically invoke dump_syms.exe on Windows when the profiler asks for symbol tables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226966/diff/4-5/

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2727fef64357
Automatically invoke dump_syms.exe on Windows when the profiler asks for symbol tables. r=dthayer,ted

Comment 32

[Code Review Bot [:reviewbot]]

Comment on attachment 8958026 [details]
Bug 1441051 - Automatically invoke dump_syms.exe on Windows when the profiler asks for symbol tables.

https://reviewboard.mozilla.org/r/226966/#review236404


Code analysis found 7 defects in this patch:
 - 7 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/extensions/ext-geckoProfiler.js:92
(Diff revision 4)
>      });
>      return promise;
>    }
>  }
>  
> +const joinBuffers = function (buffers) {

Error: Unexpected space before function parentheses. [eslint: space-before-function-paren]

::: browser/components/extensions/ext-geckoProfiler.js:174
(Diff revision 4)
> +    if (varname.toLowerCase() == name.toLowerCase()) {
> +      return value;
> +    }
> +  }
> +  return undefined;
> +}

Error: Missing semicolon. [eslint: semi]

::: browser/components/extensions/ext-geckoProfiler.js:193
(Diff revision 4)
> +  if (programFilesX86Path) {
> +    // Check if we have vswhere. This is a utilty that gets installed into a
> +    // fixed path and can be used to look up the actual location of the Visual
> +    // Studio installation. It's available starting with VS2017.
> +    const vswherePath = OS.Path.join(programFilesX86Path,
> +      "Microsoft Visual Studio", "Installer", "vswhere.exe");

Error: Expected indentation of 37 spaces but found 6. [eslint: indent]

::: browser/components/extensions/ext-geckoProfiler.js:199
(Diff revision 4)
> +    const args = [
> +      "-products", "*",
> +      "-latest",
> +      "-requires", "Microsoft.VisualStudio.Component.VC.Tools.x86.x64",
> +      "-property", "installationPath",
> +      "-format", "value"

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/ext-geckoProfiler.js:208
(Diff revision 4)
> +        (await runCommandAndGetOutputAsString(vswherePath, args)).trim();
> +      if (vsInstallationPath) {
> +        const diaSDKPath = OS.Path.join(vsInstallationPath, "DIA SDK");
> +        return [
> +          OS.Path.join(diaSDKPath, "bin"),
> +          OS.Path.join(diaSDKPath, "bin", "amd64")

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/ext-geckoProfiler.js:220
(Diff revision 4)
> +    }
> +  }
> +  // Add no extra DLL search paths and hope that dump_syms.exe is going to
> +  // succeed regardless.
> +  return [];
> +}

Error: Missing semicolon. [eslint: semi]

::: browser/components/extensions/ext-geckoProfiler.js:231
(Diff revision 4)
> +  env.PATH = existingPaths.concat(extraPaths).join(";");
> +  const opts = {
> +    command: dumpSymsPath,
> +    arguments: [debugPath],
> +    environment: env,
> +    stderr: "pipe"

Error: Missing trailing comma. [eslint: comma-dangle]

Comment 33

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/2727fef64357