Skip to content

Possibly deoptimizing use of arguments in libs #10323

Closed
@vsemozhetbyt

Description

@vsemozhetbyt

I've started to read the v8-bailout-reasons, in particular the "Bad value context for arguments value" part. @vhf refers to this part of more detailed research, where we can find some safety rules about arguments. So I've tried to scan Node.js libs for possible violations of these rules. Here are the suspicious fragments I've found.

  1. Leaking arguments:

domain.js: function Domain.prototype.intercept leaks here. (Possibly is not worth the efforts; see this comment)
domain.js: function Domain.prototype.bind leaks here. (Possibly is not worth the efforts; see this comment)
internal/process.js: function setupConfig leaks here. (Possibly is not worth the efforts; see this comment)
internal/util.js: function exports._deprecate leaks here. (It does not cause deopt; see this PR)

  1. Using arguments[index] with a possibility of index to be out of the arguments bounds.

This is more difficult case to check, but I've inferred possible arguments.length variability from docs and code comments. Maybe I'm wrong for some or all cases. I will refer to a first possible violation in a function, the function can contain more.

child_process.js: function normalizeExecArgs uses possible out of bounds index(es) from here. (Fixed)
child_process.js: function exports.execFile uses possible out of bounds index(es) from here. (Fixed)
child_process.js: function normalizeSpawnArguments uses possible out of bounds index(es) from here. (Fixed)

dgram.js: function Socket.prototype.bind uses possible out of bounds index(es) from here. (Fixed)

events.js: function EventEmitter.prototype.emit uses possible out of bounds index(es) from here.
There is a sign of awareness in the code, but maybe ES6 makes it possible to resolve this difficulty in the EventEmitter.prototype.emit itself.
(Fixed)

fs.js: all the functions with maybeCallback or makeCallback calls can use possible out of bounds index(es). (they are not called without parameters; see this PR)

util.js: function inspect uses possible out of bounds index(es) from here. (Fixed)

_debugger.js: function Interface.prototype.scripts uses possible out of bounds index(es) from here. (Possibly is not worth the efforts; see this comment)
_debugger.js: function Interface.prototype.watchers uses possible out of bounds index(es) from here. (Possibly is not worth the efforts; see this comment)

I am not sure about any of these cases: if they are real violations, if these violations have any real impact on the performance and so on. And unfortunately, I have not sufficient knowledge of all the codebase system to propose any changes here. So take this as a start point memo for contributors who consider it to be worth any attention and close it if this is some needless overagitating.

P.S. Part 2.

Metadata

Metadata

Assignees

No one assigned

    Labels

    good first issueIssues that are suitable for first-time contributors.lib / srcIssues and PRs related to general changes in the lib or src directory.performanceIssues and PRs related to the performance of Node.js.v8 engineIssues and PRs related to the V8 dependency.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions