Fallback to no-extra-bindings-mode in Debugger.Object.prototype.executeInGlobalWithBindings if possible
Categories
(Core :: JavaScript Engine, task, P2)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox118 | --- | fixed |
People
(Reporter: arai, Assigned: arai)
References
(Blocks 2 open bugs, Regressed 1 open bug)
Details
Attachments
(6 files, 3 obsolete files)
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review |
Debugger.Object.prototype.executeInGlobalWithBindings has the following problems:
- bug 1841964 / bug 1842441: not-intuitive variable access when syntactic variable conflicts with bindings
- bug 793345: slow performance due to not-optimized variable access
Both comes from the non-syntactic scope that causes the access to global/unqualified names getting de-optimized.
Given Debugger.Object.prototype.executeInGlobalWithBindings performance is important for DevTools and WebDriver, we could add dedicate mode and code path for this specific case.
We could apply some optimization given:
- the list of extra bindings is known at the point of compilation, and the list doesn't change for the entire execution
- if the conflict happens, users will most likely expect syntactic variables in their code always shadows the extra bindings
so, the following optimization can be applied:
- syntactic global variables and global lexicals access can use regular opcodes (
JSOp::GetGNameetc) - unqualified names which doesn't appear in the extra bindings don't need to look into the extra bindings's environment
this will require the following:
- extra compilation/execution mode for "global script with extra bindings"
- a new scope and environment kind other than
WithEnvironmentObject, to skip it during unqualified names lookup
possible limitation is the case where the code contains eval, which will need to keep using the current code path.
|
Assignee | |
Comment 1•1 year ago
|
||
|
|
Assignee | |
Comment 2•1 year ago
|
||
Attached WIP, which seems to solve both bug 1841964 and bug 793345.
the environment/scope handling is still rough and it needs some more work.
|
|
Assignee | |
Comment 3•1 year ago
|
||
the main issue with the environment/scope handling is the JIT interaction with non-syntactic scope or the new environment type.
possible options I can think of are:
- (a) use dedicate environment/scope for extra-bindings, and add special handling for extra-bindings to baseline, Ion, bailout code, etc
- (b) use existing non-syntactic scope (with a tweak to omit conflict bindings), but only if any extra-binding is used (or possibly used via
eval). if if none of extra-bindings are accessed, compile the code as regular global script
(b) is a quick workaround for the slow-ness, but users may find it strange that using $ etc makes the script slow.
(a) is more straight-forward solution but needs extra work.
|
|
Assignee | |
Comment 4•1 year ago
|
||
|
|
Assignee | |
Comment 5•1 year ago
|
||
attached another WIP, which keeps using WithEnvironmentObject, but without conflicting bindings, and also fallback to regular global script if no binding is used in the script.
this solves bug 1841964, and also solves bug 793345 if the script doesn't use the extra bindings.
|
|
Assignee | |
Comment 6•1 year ago
|
||
So, I have a question regarding DevTools usage and WebDriver usage.
If the performance optimization for Debugger.Object.prototype.executeInGlobalWithBindings works only when the script has no reference to the extra bindings, how much beneficial would it be?
For example, if the reference for the extra binding always exists in the script, this won't improve any performance.
The detail about the "when the script has no reference to the extra bindings" is the following:
The following is an example where the script has a reference to an extra binding X, and the optimization doens't apply.
executeInGlobalWithBindings(`
console.log(X);
for (var i = 0; i < 10000000; i++) {} // slow
`, { X: 10 });
The condition is shared across all scripts (top-level, and all functions).
In the following case, only the function f has reference to X, but in this case, the optimization doesn't apply also to top-level and function g.
(although, the problem doesn't affect function script too much (see bug 793345 comment #25)
executeInGlobalWithBindings(`
function f() { console.log(X); }
function g() { }
for (var i = 0; i < 10000000; i++) {} // slow
`, { X: 10 });
The existence of the reference affects even if the execution doesn't reach the reference, and the optimization doesn't apply.
executeInGlobalWithBindings(`
if (some_falsy_condition) {
console.log(X);
}
for (var i = 0; i < 10000000; i++) {} // slow
`, { X: 10 });
Also, eval can access the binding, and the existence of direct eval prevents the optimization:
executeInGlobalWithBindings(`
eval(unknown_code);
for (var i = 0; i < 10000000; i++) {} // slow
`, { X: 10 });
Otherwise, the optimization works.
executeInGlobalWithBindings(`
for (var i = 0; i < 10000000; i++) {} // fast
`, { X: 10 });
|
||
Comment 7•1 year ago
|
||
I think that for WebDriver it won't make much difference because the only time we are using executeInGlobalWithBindings, it's with an IIFE and that seems optimized already.
However for devtools I think this would be super beneficial. If I remember correctly, the bindings we inject are all the helpers that you expect to have available in the console (eg $, $$ ... I guess the full list is more or less: devtools/server/actors/webconsole/commands/manager.js). I won't say it's rare to use those helpers, but I think if we can get a performance improvement for all the DevTools console scripts which don't happen to use the helpers it would be great.
Unless I'm missing something and we somehow end up automatically referencing some of those bindings? If the patch is ready to be tested, simply testing the example from Bug 793345 should give us a good idea whether this is useful or not.
|
|
Assignee | |
Comment 8•1 year ago
|
||
Thank you for your input :)
Okay, I'll look into the prototype B approach first.
with the patch, bug 793345 testcase runs in the same speed between the web content and devtools console.
|
|
Assignee | |
Comment 9•1 year ago
|
||
Depends on D184195
|
|
Assignee | |
Comment 10•1 year ago
|
||
Depends on D184196
|
|
Assignee | |
Comment 11•1 year ago
|
||
Depends on D184197
|
|
Assignee | |
Comment 12•1 year ago
|
||
Depends on D184198
|
||
Updated•1 year ago
|
|
|
Assignee | |
Comment 13•1 year ago
|
||
Depends on D184199
|
|
||
Updated•1 year ago
|
|
|
Assignee | |
Updated•1 year ago
|
|
|
Assignee | |
Comment 15•1 year ago
|
||
Actually, this doesn't work with devtools command (e.g. :screenshot), because it relies on the existing shadowing behavior (bindings shadows globals).
// When we are running command via `:` prefix, no user code is being ran and only the command executes,
// so always expose the commands as the command will try to call its JavaScript method (see getEvalInput).
// Otherwise, when we run user code, we want to avoid overriding existing symbols with commands.
// Also ignore commands which can only be run with the `:` prefix.
if (
!isCmd &&
the following test fails with the patches because global screenshot shadows the extra binding screenshot
function screenshot() {
...
const command = `:screenshot --clipboard`;
|
|
Assignee | |
Comment 16•1 year ago
|
||
if :screenshot --clipboard is entered to the console, it's converted into screenshot({"clipboard":true}) and evaluated with executeInGlobalWithBindings.
possible workaround:
- (a) Add an option to
executeInGlobalWithBindingsto keep using the old behavior (CompileGlobalScriptwith non-syntactic scope), and use it if it was command - (b) Avoid the collision by looking for not-colliding name
- (b.1) add prefix to the function to avoid conflict: e.g.
devtools_0_screenshot({"clipboard":true}) - (b.2) use
with: e.g.with (devtools_bindings_0) { screenshot({"clipboard":true}); }
- (b.1) add prefix to the function to avoid conflict: e.g.
- (c) do not use
executeInGlobalWithBindingsfor command
|
|
Assignee | |
Comment 17•1 year ago
|
||
:nchevobbe, can I have your input here, how to approach the above issue?
can the command be implemented in different way?
|
|
Assignee | |
Comment 18•1 year ago
|
||
| Comment hidden (obsolete) |
|
|
||
Updated•1 year ago
|
|
|
Assignee | |
Comment 20•1 year ago
|
||
|
|
Assignee | |
Comment 21•1 year ago
|
||
Added yet another patch with (c) way. I think the command-only case doesn't have to use the debugee, given all parameters are known, and don't come from debuggee.
(In reply to Tooru Fujisawa [:arai] from comment #21)
Added yet another patch with (c) way. I think the command-only case doesn't have to use the debugee, given all parameters are known, and don't come from debuggee.
Sorry for the late response, I was crafting a response but seems like you went with something I was going to suggest :) So yes, I agree, we shouldn't need to involve the debuggee for those commands.
|
||
Comment 23•1 year ago
|
||
|
|
Assignee | |
Comment 24•1 year ago
|
||
Just realized that there are more bindings that may affect other feature.
I'll ask for backout and re-think about the binding shadowing.
// '_self' refers to the JS object references via options.selectedObjectActor.
// This isn't exposed on typical console evaluation, but only when "Store As Global"
// runs an invisible script storing `_self` into `temp${i}`.
if (bindSelf) {
bindings._self = bindSelf;
}
// Log points calls this method from the server side and pass additional variables
// to be exposed to the evaluated JS string
if (options.bindings) {
bindings = { ...bindings, ...options.bindings };
}
|
|
||
Comment 25•1 year ago
|
||
|
|
Assignee | |
Comment 26•1 year ago
|
||
There are some cases that's affected by this change, which results in breaking the behavior if the page as a conflicting binding.
Also there are some cases that's already affected by conflicting binding, because of existing shadowing behavior.
It seems to be that, there are 3 types of bindings:
- (A) Not used by internal code, and provided only for users
- This kind of bindings can always be shadowed
- (B) Used only by internal code
- This kind of bindings shouldn't be shadowed. Otherwise the feature breaks
- (C) Sometimes used by internal code, but also provided for users
- This is complex case. If this is used by internal code, the binding shouldn't be shadowed, but if used by users, this should be shadowed
(A) case works perfectly with the patch's change.
(B) case doesn't work with the patch's change, and we'll need another way, such like, having 2 set of bindings, one for (A) and one for (B), and (A) gets shadowed but (B) doesn't get shadowed.
(C) case would need some kind of refactoring on DevTools side, not to rely on the current bindings, but to introduce (B)-kind bindings.
:nchevobbe, can I have your input what to do here?
Details of the bindings in the following:
This change affects when evalWithDebugger is called without options.frameActor provided.
If options.frameActor is provided, frame.evalWithBindings is used instead and the provided bindings shadows any global/local variables.
Each related consumer listed below.
Sections with "AFFECTED" are affected by this patch.
Sections with "PRE-EXISTING" have similar problem which is introduced by this patch.
_self binding
_self binding is defined when options.selectedObjectActor is provided.
const { dbgGlobal, bindSelf } = getDbgGlobal(options, dbg, webConsole);
...
if (bindSelf) {
bindings._self = bindSelf;
function getDbgGlobal(options, dbg, webConsole) {
...
const jsVal = actor instanceof LongStringActor ? actor.str : actor.rawValue();
if (!isObject(jsVal)) {
return { bindSelf: jsVal, dbgGlobal };
...
const bindSelf = dbgGlobal.makeDebuggeeValue(jsVal);
return { bindSelf, dbgGlobal };
Also, options.selectedObjectActor is specified when ScriptCommand.execute is called with options.selectedObjectActor or options.selectedNodeActor or options.frameActor.
async execute(expression, options = {}) {
const {
selectedObjectActor,
selectedNodeActor,
frameActor,
selectedTargetFront,
} = options;
...
const selectedActor =
selectedObjectActor || selectedNodeActor || frameActor;
...
selectedObjectActor,
"Store as global variable" menu item (AFFECTED)
This uses commands.scriptCommand.execute with options.selectedObjectActor and without options.frameActor,
thus this uses executeInGlobalWithBindings path.
The expression relies on _self binding, which hadn't been shadowed by global, and
this feature breaks with the change.
tempN is directly defined on this, and not affected by the bindings change.
const evalString = `{ let i = 0;
while (this.hasOwnProperty("temp" + i) && i < 1000) {
i++;
}
this["temp" + i] = _self;
"temp" + i;
}`;
...
const res = await commands.scriptCommand.execute(evalString, {
selectedObjectActor: actor,
disableBreaks: true,
});
"Copy Object" menu item (AFFECTED/PRE-EXISTING)
This also uses commands.scriptCommand.execute with options.selectedObjectActor and without options.frameActor,
thus this uses executeInGlobalWithBindings path.
The expression also relies on _self binding, which hadn't been shadowed by global.
This feature breaks when _self binding gets shadowed.
Also, the expression relies on copy binding, which had been shadowed by global.
This is pre-existing issue, but the behavior doesn't change with the bindings change.
const res = await commands.scriptCommand.execute("copy(_self)", {
selectedObjectActor: actor,
disableBreaks: true,
});
options.bindings
Caller can provide extra bindings by options.bindings.
if (options.bindings) {
bindings = { ...bindings, ...options.bindings };
logEvent on worker
On worker, logEvent uses _consoleActor.evaluateJS without options.frameActor.
(on non-worker, this uses frame.evalWithBindings and is not affected)
function logEvent({ threadActor, frame, level, expression, bindings }) {
...
const displayName = formatDisplayName(frame);
...
if (isWorker) {
threadActor._parent._consoleActor.evaluateJS({
text: `console.log(...${expression})`,
bindings: { displayName, ...bindings },
url: sourceActor.url,
lineNumber: line,
disableBreaks: true,
});
return undefined;
}
...
completion = frame.evalWithBindings(
...
);
BreakpointActor.hit (AFFECTED)
This uses [${logValue}] expression where logValue is options.logValue of the breakpoint.
const { condition, logValue } = this.options || {};
...
return logEvent({
threadActor: this.threadActor,
frame,
level: "logPoint",
expression: `[${logValue}]`,
});
addBreakpointAtLine uses displayName as logValue, which hadn't been shadowed by global, and
this feature breaks with the change if this is used on worker.
export function addBreakpointAtLine(line, shouldLog = false, disabled = false) {
...
options.logValue = "displayName";
ThreadActor._pauseAndRespondEventBreakpoint (AFFECTED)
This relies on _event binding which is provided via options.bindings, which hadn't been shadowed by global, and
this feature breaks with the change if this is used on worker.
return logEvent({
...
expression: `[_event]`,
bindings: { _event: frame.arguments[0] },
Other observation
"Use in Console" menu item (NOT-AFFECTED/PRE-EXISTING)
Relies on $0, which had already been shadowed by global,
This is pre-existing issue, but the behavior doesn't change with the bindings change.
tempN is directly defined on window, and not affected by the bindings change.
const evalString = `{ let i = 0;
while (window.hasOwnProperty("temp" + i) && i < 1000) {
i++;
}
window["temp" + i] = $0;
"temp" + i;
}`;
...
const res = await this.toolbox.commands.scriptCommand.execute(evalString, {
selectedNodeActor: this.selection.nodeFront.actorID,
disableBreaks: true,
});
Thanks for the detailed information arai.
My initial feeling is that all those internal methods shouldn't have to use script evaluation to work, and would rather have dedicated webconsole actor methods that would use the Debugger API to achieve what they should do.
I think the disableBreaks option (which Alex added not long ago), does highlight this. We have this special flag for internal features, and so it's a bit of a smell, showing the evaluation isn't the appropriate mean.
With that being said, I'm wondering if we could use that to our advantage for now and prevent the bindings to be shadowed when disableBreaks is true? I feel like this would work here but maybe I'm missing something?
|
|
Assignee | |
Comment 28•1 year ago
|
||
so, the idea is to keep the existing shadowing behavior (extra bindings shadows global variables) if options.disableBreaks is true, right?
I'll look into implementing the switch into executeInGlobalWithBindings.
(In reply to Tooru Fujisawa [:arai] from comment #28)
so, the idea is to keep the existing shadowing behavior (extra bindings shadows global variables) if
options.disableBreaksis true, right?
Correct. I think this should work (disableBreaks is only false on actual (webconsole) user evaluation)
I'll look into implementing the switch into
executeInGlobalWithBindings.
Thanks a lot!
|
|
||
Updated•1 year ago
|
|
|
||
Comment 30•1 year ago
|
||
|
||
Comment 31•1 year ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/77899a08642e
https://hg.mozilla.org/mozilla-central/rev/ceb7ff9dbef9
https://hg.mozilla.org/mozilla-central/rev/8bf98957d7ab
https://hg.mozilla.org/mozilla-central/rev/81b23bbbd047
https://hg.mozilla.org/mozilla-central/rev/a77d6cfde62d
https://hg.mozilla.org/mozilla-central/rev/07943b119126
Description
•