Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Demangle symbols when reading them out of the symbol table. #1203

Merged
merged 1 commit into from
Sep 19, 2018

Conversation

mstange
Copy link
Contributor

@mstange mstange commented Aug 19, 2018

Doing demangling in perf.html has the following advantages:

  • If we add code to Firefox that dumps symbol tables from binaries (which I'm currently doing for Firefox on Android in order to get symbols for Android system libraries), Firefox doesn't have to include a demangler. This saves on code size.
  • Mangled names are usually a bit shorter than demangled names. The symbol tables we store will be a bit more compact if we don't require the strings in them to be in a demangled form.
  • Demangling takes a bit of time. Only demangling the strings that we actually need to symbolicate a given profile saves time by not demangling everything else.

This also adds our first wasm module and updates the configuration so
that that's possible.
I've used wasm-pack to publish a gecko-profiler-demangle module to npm
instead of copying the generated code into this repository. I'm not sure
if that was a good or a bad idea; I mostly did it so that our Flow and
ESLint rules wouldn't be applied to the wasm-bindgen generated code.

I've also needed to autogenerate a flow libdef for this module.
wasm-bindgen doesn't create those automatically yet:
rustwasm/wasm-bindgen#180

I could have created one manually and included it in the published
package, but I don't know if wasm-pack would have included my additional
files in the published package (via wasm-pack publish).


Todo:

  • get tests running again - they fail because of the async import(), Our test infrastructure can't deal with async import() #1204
  • [ ] add tests that verify that things get demangled (impossible because we don't have the demangler in the test suite. We could mock some fake demangling and test that it happens when getting symbols out of symbol tables, but it would be silly.)

@gregtatum gregtatum changed the title [WIP] Demangle symbols when reading them out of the symbol table. [wip] Demangle symbols when reading them out of the symbol table. Aug 22, 2018
@mstange mstange force-pushed the demangle branch 2 times, most recently from cd0e158 to 1655f39 Compare August 23, 2018 18:26
@mstange mstange requested a review from gregtatum August 23, 2018 18:35
@mstange mstange self-assigned this Aug 23, 2018
@mstange
Copy link
Contributor Author

mstange commented Aug 23, 2018

This breaks the production build due to NekR/offline-plugin#408.

@mstange mstange force-pushed the demangle branch 3 times, most recently from 274144a to 1e68fd6 Compare September 7, 2018 21:26
@mstange mstange changed the title [wip] Demangle symbols when reading them out of the symbol table. Demangle symbols when reading them out of the symbol table. Sep 7, 2018
@codecov
Copy link

codecov bot commented Sep 7, 2018

Codecov Report

Merging #1203 into master will increase coverage by <.01%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1203      +/-   ##
==========================================
+ Coverage   77.82%   77.82%   +<.01%     
==========================================
  Files         151      151              
  Lines       10023    10025       +2     
  Branches     2449     2449              
==========================================
+ Hits         7800     7802       +2     
  Misses       2003     2003              
  Partials      220      220
Impacted Files Coverage Δ
src/index.js 0% <0%> (ø) ⬆️
src/profile-logic/symbol-store.js 93.82% <100%> (+0.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5498749...c4c915c. Read the comment docs.

@mstange
Copy link
Contributor Author

mstange commented Sep 7, 2018

IT'S GREEEN!!!!

Copy link
Member

@gregtatum gregtatum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this works for me! I left some comments that I feel should be addressed before landing, but I don't think they need a re-review.

I also looked at the gecko-profiler-demangle code which looked decently straightforward for the needs here.

src/index.js Outdated Show resolved Hide resolved
// wasm-bindgen), when targeting the browser + webpack, generates an ES6 module
// that node cannot deal with. Most importantly, it uses the syntax
// "import * as wasm from './gecko_profiler_demangle_bg';" in order to load
// the wasm module, which is currently only supported by webpack.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a bug somewhere to track this feature? If so it would be nice to link it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added:

// The long-term path to make this work correctly is to wait for node to
// support ES6 modules (and WASM as ES6 modules) natively [1]. It's possible
// that in the medium term, wasm-bindgen will get support for outputting JS
// files which work in both webpack and in node natively [2].
// [1] https://medium.com/@giltayar/native-es-modules-in-nodejs-status-and-future-directions-part-i-ee5ea3001f71
// [2] https://github.com/rustwasm/wasm-bindgen/issues/233

package.json Outdated Show resolved Hide resolved
This also adds our first wasm module and updates the configuration so
that that's possible.
I've used wasm-pack to publish a gecko-profiler-demangle module to npm
instead of copying the generated code into this repository. I'm not sure
if that was a good or a bad idea; I mostly did it so that our Flow and
ESLint rules wouldn't be applied to the wasm-bindgen generated code.

I've also needed to autogenerate a flow libdef for this module.
wasm-bindgen doesn't create those automatically yet:
rustwasm/wasm-bindgen#180

I could have created one manually and included it in the published
package, but I don't know if wasm-pack would have included my additional
files in the published package (via `wasm-pack publish`).
@mstange mstange merged commit 2453b35 into firefox-devtools:master Sep 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants