Closed Bug 1441051 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox60 wontfix, firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: xidorn, Assigned: mstange)

Details

(Whiteboard: [perf-tools])

Attachments

(1 file)

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.
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.
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...
Why do you need to run buildsymbols? I don't think that's necessary on mac and linux, why is it necessary on windows?
To make Gecko Profiler useful on a local build.
Let me rephrase: why does the profiler need you to run buildsymbols on windows, when, afaik, it doesn't on other platforms?
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)
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.
(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
(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?
Flags: needinfo?(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.
Flags: needinfo?(mstange)
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!
Product: Core → Firefox Build System
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
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
Jeff found an MIT-licensed C++ version at https://github.com/rui314/msvc-demangler . I might take a stab at converting it to rust.
(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
> https://github.com/nico/demumble

That one uses Wine's LGPL code.
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.
Summary: Add option to update symbols automatically after `mach build [binary]` → Remove the need to run `mach buildsymbols` after `mach build [binary]` in order to get profiles with symbols for local Windows builds
Xidorn, could you try this patch out?
Flags: needinfo?(xidorn+moz)
Yeah, it works for me. Thanks.
Flags: needinfo?(xidorn+moz)
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.
Attachment #8958026 - Flags: review?(ted) → review-
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 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
Attachment #8958026 - Flags: review?(dothayer) → review+
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+.
Attachment #8958026 - Flags: review- → review+
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Whiteboard: [perf-tools]
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 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.
Pushed by mstange@themasta.com:
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 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]
https://hg.mozilla.org/mozilla-central/rev/2727fef64357
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.