Repository navigation
Possibly deoptimizing use of arguments in libs #10323
Description
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.
- Leaking
arguments:
domain.js: function Domain.prototype.intercept leaks here.
domain.js: function Domain.prototype.bind leaks here.
internal/process.js: function setupConfig leaks here.
internal/util.js: function exports._deprecate leaks here.
- Using
arguments[index]with a possibility ofindexto be out of theargumentsbounds.
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.
child_process.js: function exports.execFile uses possible out of bounds index(es) from here.
child_process.js: function normalizeSpawnArguments uses possible out of bounds index(es) from here.
dgram.js: function Socket.prototype.bind uses possible out of bounds index(es) from here.
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.
fs.js: all the functions with maybeCallback or makeCallback calls can use possible out of bounds index(es).
util.js: function inspect uses possible out of bounds index(es) from here.
_debugger.js: function Interface.prototype.scripts uses possible out of bounds index(es) from here.
_debugger.js: function Interface.prototype.watchers uses possible out of bounds index(es) from here.
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.
Activity
not-an-aardvark commentedon Dec 18, 2016
I'm not familiar enough with V8 to know whether these uses of
argumentscause a noticeable performance drop, but if we conclude that they are worth avoiding, it seems like it should be possible to make a linter rule for them.vsemozhetbyt commentedon Dec 18, 2016
Eslint has a rather radical rule for all
argumentsusecases. However, the mentioned researches propose less drastic workarounds. For example, index checking,argumentscopying and.apply()mediation are already used in many lib places.vhf commentedon Dec 19, 2016
Interesting, nice findings @vsemozhetbyt .
Fixing the possible out of bound access to the
argumentsobject might be worth it. I glanced at the other things you mentioned and it might be either complicated to fix or not worth the effort or both.I'm not familiar with node internals so I might be wrong, but here is an example:
setupConfigleaksarguments. It's a tiny utility method and I'm pretty sure its use is extremely seldom. I'm pretty sure this function not being optimized cannot have any sort of significant performance impact.What might be slightly more interesting in terms of performances, but might break some people's code, would be to set
process.icu_data_dirtonullinstead ofdeleteing it. This way theprocessobject will stay fast (because ofdeleteit currently becomes a dictionary mode object and we can assume this slow object will be reused in people's code).vsemozhetbyt commentedon Dec 19, 2016
@vhf Thank you for the answer.
As I have said, I'm also not aware of all the system. For example, the mentioned
setupConfig()function is exported from the internal lib and it needs to be considered in addition how many times it can be called in all the lib system and how many times it can be triggered by various user code. (By the way, it seems you have used by accident the link to the documented property of another lib of the same name instead of the function of the internal lib).And thank you for the second comment. It will need more checks for other similar cases. Is this feature connected with v8 hidden classes you've described recently? I've also read about it in another article (chapter "Objects")
bnoordhuis commentedon Dec 19, 2016
I suspect the only functions where it could make a material difference are
EventEmitter#emit()andutil.inspect(), the others are unlikely to register under normal conditions.Fixing the other functions might still be worthwhile from a code hygiene perspective, but not if it significantly affects complexity or readability.
Fishrock123 commentedon Dec 19, 2016
(If you'd like to help fix this look here.)
My assessment:
Domain.prototype.intercept- ¯\_(ツ)_/¯Domain.prototype.bind- ¯\_(ツ)_/¯setupConfig(... reallyv8BreakIterator) - ¯\_(ツ)_/¯exports._deprecate-child_process.js>normalizeExecArgs-child_process.js>exports.execFile- Are you sure this can happen out of bounds? Probably fix.child_process.js>normalizeSpawnArgs-dgram.js>Socket.prototype.bind-events.js>EventEmitter.prototype.emit- ❗️Almost certainly important. Check for no args passed.fsI didn't check, but soundsutil.js>inspect. - ❗️Almost certainly important. Check arg counts in these._debugger.js- ¯\_(ツ)_/¯vsemozhetbyt commentedon Dec 19, 2016
@Fishrock123
Well, suppose we have only the first argument (
file) set. Soarguments.lengthis1(and onlyarguments[0]is set). So all theseif's are skipped andposremains1andcallbackremainsundefined. Soarguments[pos]here isarguments[1], i.e. out of bounds. Maybe I miss something.mgol commentedon Dec 23, 2016
Do rest parameters have the same deopt problems as the
argumentsobject? Are there any reasons why Node still usesargumentswhen it can use the full ES6 power?CapacitorSet commentedon Dec 25, 2016
@mgol, rest parameters are a known Crankshaft bailout (deoptimization) reason at this time.
bmeurer commentedon Jan 1, 2017
@mgol Rest parameters force optimization via TurboFan, which should be fine in many cases, esp. with V8 5.6 and later (not yet available for Node.js tho).
bmeurer commentedon Jan 1, 2017
@Fishrock123
EventEmitter.prototype.emitis pretty serious, esp. since with newer versions of V8, Crankshaft will disable optimization for the whole function (due a correctness fix wrt. Crankshaft's handling of arguments). See crbug.com/v8/3829 for more information on this particular magic.18 remaining items
vsemozhetbyt commentedon Feb 2, 2017
The initial comment is updated considering this opinion and all the committed fixes.
fs: : avoid out-of-bounds arguments index access
dgram: fix possibly deoptimizing use of arguments
dgram: fix possibly deoptimizing use of arguments
dgram: fix possibly deoptimizing use of arguments
child_process: fix deoptimizing use of arguments
vsemozhetbyt commentedon Mar 2, 2017
It seems all the mentioned cases have been fixed or reasonably rejected, so this can be closed.
dgram: fix possibly deoptimizing use of arguments
dgram: fix possibly deoptimizing use of arguments
dgram: fix possibly deoptimizing use of arguments
dgram: fix possibly deoptimizing use of arguments
child_process: fix deoptimizing use of arguments
child_process: fix deoptimizing use of arguments
child_process: fix deoptimizing use of arguments