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

Enhance TFramedTransport performance for nodejs #2483

Merged
merged 1 commit into from
Apr 21, 2022

Conversation

wujjpp
Copy link
Contributor

@wujjpp wujjpp commented Dec 2, 2021

Enhance TFramedTransport performance for nodejs

  1. use array.push instread of Array.concat, array.push is more effective than Array.concat, see benchmark results
  2. reduce data copy action for improving performance.
const Benchmark = require('benchmark');

const buffer = Buffer.alloc(1024)
buffer.fill('A')
const loop = 1024;

const suite = new Benchmark.Suite()

suite
  .add('Buffer.concat', function () {
    let target = Buffer.alloc(0)
    for(let i = 0; i < loop; ++i){
      target = Buffer.concat([target, buffer], target.length + buffer.length)
    }
  })
  .add('Array.push', function () {
    const target = []
    for(let i = 0; i < loop; ++i){
      for(let j = 0; j < buffer.length; ++j){
        target.push(buffer[i])
      }
    }
  })
  .on('cycle', function(event) {
    console.log(String(event.target));
  })
  .on('complete', function() {
    console.log('Fastest is ' + this.filter('fastest').map('name'));
    // console.log(this)
  })
  .run({ async: true })
results:
Buffer.concat x 6.72 ops/sec ±21.71% (24 runs sampled)
Array.push x 47.35 ops/sec ±1.56% (61 runs sampled)
Fastest is Array.push

@wujjpp wujjpp changed the title Buffer.concat has performance issue Performance issue about TFramedTransport Dec 2, 2021
@wujjpp wujjpp changed the title Performance issue about TFramedTransport nodejs performance issue about TFramedTransport Dec 2, 2021
@wujjpp wujjpp changed the title nodejs performance issue about TFramedTransport Enhance TFramedTransport performance for nodejs Dec 2, 2021
callback(new TFramedTransport(frame), seqid);

data = residual;
residual = null;
}
Copy link
Member

Choose a reason for hiding this comment

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

What happened to "data"?

Copy link
Contributor Author

@wujjpp wujjpp Dec 13, 2021

Choose a reason for hiding this comment

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

Combined to residual, then if condition matched, splice(4) and splice(frameSize)

var residual = null;

var residual = [];

return function(data) {
Copy link
Member

@Jens-G Jens-G Dec 13, 2021

Choose a reason for hiding this comment

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

Nope, still says "data".

return function(data) {

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Apr 16, 2022
@Jens-G Jens-G merged commit 0dc5298 into apache:master Apr 21, 2022
@wujjpp wujjpp deleted the thrift-nodejs-performance branch October 26, 2022 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants