nodejs / nodejs/performance

Optimizing internal argument validation functions

Open
#140 11 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Shell
Stars
399
Forks
11
Avg merge
29m
Merged PRs (30d)
1

Description

Some internal functions used to validate arguments, like validateString, seem to be called all over the place throughout the codebase, so making these calls as fast as possible may be worth it.

IMO the way they are being used today is probably a lot less efficient than it could be, though without measuring the impact of the change it's hard to say what it will be.

Let's take a look at a scenario, and at how to make it faster:

  1. For example validateString is being called here to validate our argument when doing crypto.createHash('sha1').
  2. validateString's definition is basically a function wrapped by hideStackFrames
  3. hideStackFrames basically makes a function that calls our function, via ReflectApply, and if an error is thrown it does something with it and re-throws it.
  4. Finally the actual body we want to execute of the validateString function just does a typeof check, and if it fails an error is thrown.

That seems a pretty convoluted way to do a typeof check basically. And I think it could be made faster, though unclear by how much, like this:

  1. Instead of writing this:
    const validateString = hideStackFrames((value, name) => {
      if (typeof value !== 'string')
        throw new ERR_INVALID_ARG_TYPE(name, 'string', value);
    });
    
    I guess we could just write this:
    const validateString = (value, name) => {
      if (typeof value !== 'string')
        throw hideStackFramesFromError(new ERR_INVALID_ARG_TYPE(name, 'string', value));
    };
    
    Basically the idea is that unless the error is actually throw, we pay absolutely 0 cost for whatever hideStackFrames does, and instead we manually call a function that does what's needed on the error. Doing it this way would be a bit more error prone, but these validation functions seem to be fairly simple, and called all over the place, so manually inserting the hideStackFramesFromError calls in the right spots shouldn't be a problem.
  2. Lastly it may be best to inline these tiny functions that are called a million times at build time, perhaps defining a dedicated function for their erroring branch, to keep the emitted code to parse minimal. So for example the definition of our validateString would become something like this instead:
    const validateString = (value, name) => {
      if (typeof value !== 'string')
        validateStringFailure(value, name);
    };
    
    And when inlined this code:
      if (!(algorithm instanceof _Hash))
        validateString(algorithm, 'algorithm');
    
    Will be transformed automatically into this:
      if (!(algorithm instanceof _Hash))
        if(typeof algorithm !== 'string')
          validateStringFailure(algorithm, 'algorithm');
    

Basically when errors won't actually be thrown, which presumably is almost always the case, we would go from 3 function calls to 1 without the inline step, and to 0 function calls with the inline step. Also the try..catch block would get deleted, that may be ~zero cost though if not triggered, I'm not sure.

I would guess that we could see a low single-digit percentage performance improvement with this optimization applied to all the validation functions, at least in some cases, because pretty much every Node API seems to call some of these, often more than once, so it seems probably a worthwhile optimization to me.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by measuring the validateString path used by crypto.createHash('sha1'), then read lib/internal/validators.js, lib/internal/errors.js, and lib/internal/crypto/hash.js. Compare the current hideStackFrames and ReflectApply flow with the proposed alternatives; done means a measured performance result and an agreed, tested optimization across the validation functions.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
performance
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.