Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Performance: Use one 24 byte random buffer instead of calling randomBytes() twice #198

Merged
merged 1 commit into from
Nov 21, 2018

Conversation

danielkhan
Copy link
Contributor

Calling randomBytes() only once to create a 24 byte buffer and converting it to hex and slicing the string then provides the same randomness while shaving off ~40% execution time.

Testcode

const crypto = require('crypto');

function headerCallTwice() {
  return {
    traceId: crypto.randomBytes(16).toString('hex'),
    spanId: crypto.randomBytes(8).toString('hex'),
    traceState: undefined
  };
}

function headerSliceBuffer() {
  const buff = crypto.randomBytes(24);
  return {
    traceId: buff.slice(0, 16).toString('hex'),
    spanId: buff.slice(16, 24).toString('hex'),
    traceState: undefined
  };
}

function headerSliceString() {
  const buff = crypto.randomBytes(24).toString('hex');
  return {
    traceId: buff.slice(0, 32),
    spanId: buff.slice(32, 48),
    traceState: undefined
  };
}

function timeFunc(f, n) {
  const hrstart = process.hrtime();
  for(let i = 0; i < n; i++) {
    f()
  }
  const hrend = process.hrtime(hrstart);
  console.info(f.name + ' Execution time (hr): %ds %dms', hrend[0], hrend[1] / 1000000)
  console.log('-----------------------');
}


timeFunc(headerCallTwice, 1000000);
timeFunc(headerSliceBuffer, 1000000);
timeFunc(headerSliceString, 1000000);

Result (Node.js v8.11.1):

headerCallTwice Execution time (hr): 5s 907.900675ms
-----------------------
headerSliceBuffer Execution time (hr): 4s 96.412333ms
-----------------------
headerSliceString Execution time (hr): 3s 654.048213ms

@danielkhan danielkhan changed the title Performance: Use one long random buffer and slice it then Performance: Use one 24 byte random buffer instead of calling randomBytes() twice Nov 21, 2018
@isaikevych isaikevych self-requested a review November 21, 2018 19:12
Copy link
Contributor

@isaikevych isaikevych left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@justindsmith Justing, could you please look at this changes?

Copy link
Contributor

@justindsmith justindsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thanks @danielkhan!

@justindsmith justindsmith merged commit 04f7649 into census-instrumentation:master Nov 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants