Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use bl for a faster delimiter parser #1693

Closed
wants to merge 1 commit into from
Closed

Conversation

reconbot
Copy link
Member

@reconbot reconbot commented Oct 5, 2018

No description provided.

@codecov
Copy link

codecov bot commented Oct 5, 2018

Codecov Report

Merging #1693 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1693      +/-   ##
==========================================
+ Coverage      64%   64.04%   +0.04%     
==========================================
  Files          22       22              
  Lines         839      840       +1     
==========================================
+ Hits          537      538       +1     
  Misses        302      302
Impacted Files Coverage Δ
packages/parser-delimiter/delimiter.js 100% <100%> (ø) ⬆️

@HipsterBrown
Copy link
Contributor

Any metrics for speed improvements? Is it possible to add some information in the description of this PR as to why bl is faster?

@reconbot
Copy link
Member Author

reconbot commented Oct 5, 2018

It copies buffers less, I think it might be slower for large delimiters but for the common case of a single byte it's doing much less work. I'm really not sure how to test it. Probably need 2-3 benchmarks for different kinds of data.

@HipsterBrown
Copy link
Contributor

Cool, just want to add some context to the PR for people curious about the addition of this dependency. 😄

@reconbot
Copy link
Member Author

reconbot commented Oct 5, 2018

I'm actually kind of worried it will be slower for some workloads. Do you have any favorite tools for testing performance?

@reconbot
Copy link
Member Author

reconbot commented Oct 6, 2018

benchmark was awesome

This was my approach, the new bl one was not faster (even though my hunch is it uses less memory), so I'm working on making bl.indexOf faster.

const Benchmark = require('benchmark')
const DelimiterParser = require('./')

const smallBuffers = new Array(1000).map(() => Buffer.concat([Buffer.alloc(14, 1), Buffer.from('\n')]))
const largeBuffers = new Array(1000).map(() => Buffer.concat([Buffer.alloc(1024, 1), Buffer.from('\n')]))
const mixedBuffers = new Array(1000).map((val, i) => Buffer.concat([Buffer.alloc((i * i * (Math.random() + 1)) % 1024, 1), Buffer.from('\n')]))

const suite = new Benchmark.Suite()

suite.add('smallBuffers', () => {
  let count = 0
  const parser = new DelimiterParser({ delimiter: '\n' })
  parser.on('data', () => count++)
  smallBuffers.map(data => parser.write(data))
  return count
})

suite.add('largeBuffers', () => {
  let count = 0
  const parser = new DelimiterParser({ delimiter: '\n' })
  parser.on('data', () => count++)
  largeBuffers.map(data => parser.write(data))
  return count
})

suite.add('mixedBuffers', () => {
  let count = 0
  const parser = new DelimiterParser({ delimiter: '\n' })
  parser.on('data', () => count++)
  mixedBuffers.map(data => parser.write(data))
  return count
})

suite
  .on('cycle', event => {
    console.log(String(event.target))
  })
  .on('complete', () => {
    console.log('done')
  })
  .run({ async: true })

@reconbot
Copy link
Member Author

reconbot commented Oct 7, 2018

Even with the bl perf improvements unless we have huge data events this isn't any faster.

@reconbot reconbot closed this Oct 7, 2018
@reconbot reconbot deleted the reconbot/bl branch October 7, 2018 17:00
@reconbot
Copy link
Member Author

reconbot commented Oct 7, 2018

smallBuffers Old x 925 ops/sec ±13.88% (76 runs sampled)
smallBuffers x 142 ops/sec ±61.00% (43 runs sampled)
mixedBuffers old x 437 ops/sec ±62.17% (40 runs sampled)
mixedBuffers x 32.56 ops/sec ±59.19% (12 runs sampled)

@lock lock bot locked as resolved and limited conversation to collaborators Apr 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants