Closed
Bug 1338914
Opened 7 years ago
Closed 7 years ago
Baldr: having dev console open at all causes eager wasm binary-to-text conversion
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: luke, Assigned: yury)
References
Details
Attachments
(1 file)
If the dev console is open at all (not just Debugger), it seems that wasm::BinaryToText() is called eagerly from within a call to DebuggerSource_getText. This should only happen when the user clicks on a wasm source in the debugger pane.
Comment 1•7 years ago
|
||
I've added a few printf to understand who was calling BinaryToText in this case. It's called by WasmCode::createText, which is called by DebuggerSourceGetTextMatcher::match, which is called by DebuggerSource_getText, which is a JS method `text` on the DebuggerSource object. http://searchfox.org/mozilla-central/search?q=source.text&case=false®exp=false&path=devtools%2F shows a probable subset of the calls to .text; looking at a few conditionals, it seems that: - it is assumed that .text is very cheap, if not free. That's not the case, here... - sometimes callers want to know if there's an actual source (by comparing source.text to a placeholder value, which is fragile), which could/should be an API provided by the debugger, maybe.
Reporter | ||
Comment 2•7 years ago
|
||
Yury: do you know why this is being called?
Flags: needinfo?(ydelendik)
Assignee | ||
Comment 3•7 years ago
|
||
Looks like there is a check at https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/utils/TabSources.js#805: function isHiddenSource(source) { // Ignore the internal Function.prototype script return source.text === "() {\n}"; } by replacing it with `source.introductionType !== "wasm" && source.text === "() {\n}";` looks like problem is going anyway.
Flags: needinfo?(ydelendik)
Reporter | ||
Comment 4•7 years ago
|
||
Given that this asks for the source of every script when the dev console is open (including for profiling) and this is a slow path in general, I was wondering if the "right" fix here is to define isHiddenSource() in a way that doesn't use source.text. Jim?
Flags: needinfo?(jimb)
Reporter | ||
Comment 5•7 years ago
|
||
Or, maybe, by the same logic, we don't want to be doing any of this at all unless the debugger tab is open?
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8840081 -
Flags: review?(shu)
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8840081 [details] Bug 1338914 - Optimize hidden/internal script detection for devtools. https://reviewboard.mozilla.org/r/114620/#review118024 I don't like the approach for the engine to mark things as "internal" because "internal" is a subjective thing. In this particular case, it seems like the devtools frontend only cares about blacklisting Function.prototype. Could you just make the Function.prototype's source's introductionType "Function.prototype", then filter on that?
Attachment #8840081 -
Flags: review?(shu) → review-
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8840081 [details] Bug 1338914 - Optimize hidden/internal script detection for devtools. https://reviewboard.mozilla.org/r/114620/#review118526
Attachment #8840081 -
Flags: review?(shu) → review+
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ydelendik
Status: NEW → ASSIGNED
Comment 10•7 years ago
|
||
Pushed by ydelendik@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8a2b1bdb1549 Optimize hidden/internal script detection for devtools. r=shu
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8a2b1bdb1549
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•7 years ago
|
Flags: needinfo?(jimb)
You need to log in
before you can comment on or make changes to this bug.
Description
•