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

RESOLVED FIXED in Firefox 54

Status

()

Core
JavaScript Engine
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: luke, Assigned: yury)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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.
(Reporter)

Comment 2

a year ago
Yury: do you know why this is being called?
Flags: needinfo?(ydelendik)
(Assignee)

Comment 3

a year 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

a year 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

a year 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

a year ago
Attachment #8840081 - Flags: review?(shu)

Comment 7

a year 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

a year 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

a year ago
Assignee: nobody → ydelendik
Status: NEW → ASSIGNED

Comment 10

a year 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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8a2b1bdb1549
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Updated

9 months ago
Flags: needinfo?(jimb)
You need to log in before you can comment on or make changes to this bug.