Closed Bug 1338914 Opened 3 years ago Closed 3 years ago

Baldr: having dev console open at all causes eager wasm binary-to-text conversion

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: luke, Assigned: yury)

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.
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&regexp=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.
Yury: do you know why this is being called?
Flags: needinfo?(ydelendik)
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)
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)
Or, maybe, by the same logic, we don't want to be doing any of this at all unless the debugger tab is open?
Attachment #8840081 - Flags: review?(shu)
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 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: nobody → ydelendik
Status: NEW → ASSIGNED
Pushed by ydelendik@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8a2b1bdb1549
Optimize hidden/internal script detection for devtools. r=shu
https://hg.mozilla.org/mozilla-central/rev/8a2b1bdb1549
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Flags: needinfo?(jimb)
You need to log in before you can comment on or make changes to this bug.