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.

Sign up for GitHub

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

Jump to bottom

[WIP] refactor: enhance decode performance #74

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[WIP] refactor: enhance decode performance #74

wants to merge 1 commit into from
+206 −193

Conversation

gxcsoccer
Copy link
Member

@gxcsoccer gxcsoccer commented May 10, 2017 •
edited
Loading

初步优化了下 number, date 和 string，性能和孝达的 fast hessian2 差不多了

  Fast hessian2 decode: number x 10,408,116 ops/sec ±1.10% (87 runs sampled)
  Fast hessian2 decode: date   x  4,139,701 ops/sec ±1.04% (88 runs sampled)
  Fast hessian2 decode: string x  1,843,122 ops/sec ±5.17% (61 runs sampled)

  js hessian2 decode: number x 11,456,920 ops/sec ±1.18% (88 runs sampled)
  js hessian2 decode: date   x  3,964,458 ops/sec ±1.14% (90 runs sampled)
  js hessian2 decode: string x  1,617,342 ops/sec ±1.08% (93 runs sampled)

@gxcsoccer gxcsoccer requested review from fengmk2 and dead-horse May 10, 2017 18:40
@mention-bot
Copy link

@gxcsoccer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dead-horse and @fengmk2 to be potential reviewers.

gxcsoccer
gxcsoccer commented May 10, 2017
View reviewed changes
lib/v1/decoder.js
@@ -92,7 +95,7 @@ proto.handleType = function(type, val, withType) {
* @api public
*/
proto.readNull = function () {
this._checkLabel('readNull', 'N');
Copy link
Member Author

Choose a reason for hiding this comment

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

如果把 readNull，readInt 等这种 api 都变成私有的，只暴露 read，那 checkLabel 这个是没必要的

gxcsoccer
gxcsoccer commented May 10, 2017
View reviewed changes
lib/v2/decoder.js
@@ -299,7 +306,7 @@ utils.addByteCodes(BYTE_CODES, [
proto.readDate = function () {
var code = this.byteBuffer.get();
if (code === 0x4a) {
return new Date(utils.handleLong(this.byteBuffer.getLong()));
return new Date(this.byteBuffer.getLong().toNumber());
Copy link
Member Author

Choose a reason for hiding this comment

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

针对 date，直接 toNumber 就好了

dead-horse
dead-horse reviewed May 11, 2017
View reviewed changes
lib/v1/decoder.js
head = this.byteBuffer.get();
l = utils.lengthOfUTF8(head);
this.byteBuffer.skip(l - 1);
ch = this.byteBuffer.get();
Copy link
Member

Choose a reason for hiding this comment

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

这个改完会有多大性能提升？

Copy link
Member Author

Choose a reason for hiding this comment

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

少了一次循环，我之前有测，==

Copy link
Member Author

Choose a reason for hiding this comment

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

优化前

hessian2 decode: string x 1,251,833 ops/sec ±1.35% (86 runs sampled)

优化后

hessian2 decode: string x 1,482,684 ops/sec ±1.29% (85 runs sampled)

@pmq20
Copy link

pmq20 commented May 11, 2017 •
edited
Loading

初步优化了下 number, date 和 string，性能和孝达的 fast hessian2 差不多了

这俩组数据要在同一台机器 && Node.js version 上跑才有意义。我试一下我的机器 && node 8-pre

@pmq20
Copy link

pmq20 commented May 11, 2017

@gxcsoccer 同一台机器，同样的 node 版本下：

  node version: v8.0.0-pre

  hessian2 decode: number x 4,721,288 ops/sec ±0.54% (87 runs sampled)
  hessian2 decode: date   x 2,599,811 ops/sec ±0.61% (92 runs sampled)
  hessian2 decode: string x   557,185 ops/sec ±1.01% (88 runs sampled)

  Fast hessian2 decode: number x 9,706,914 ops/sec ±1.41% (84 runs sampled)
  Fast hessian2 decode: date   x 3,861,312 ops/sec ±1.18% (86 runs sampled)
  Fast hessian2 decode: string x 1,671,423 ops/sec ±5.69% (60 runs sampled)

fengmk2
fengmk2 reviewed May 11, 2017
View reviewed changes
lib/v1/decoder.js
@@ -47,7 +47,10 @@ proto._addRef = function (obj) {
* @api public
*/
proto.init = function (buf) {
this.byteBuffer = ByteBuffer.wrap(buf);
this.byteBuffer._bytes = buf;
Copy link
Member

Choose a reason for hiding this comment

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

为何要使用这种私有 api？

Copy link
Member

Choose a reason for hiding this comment

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

this.byteBuffer.reset(buf) 这种不是更好？

Copy link
Member Author

Choose a reason for hiding this comment

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

reset 没有入参，这里是 WIP，还不是最终版本，先找到优化点，后面可以针对性的对 api 做调整

ByteBuffer.prototype.reset = function () {
  this._offset = 0;
};

@fengmk2
Copy link
Member

fengmk2 commented May 11, 2017

@gxcsoccer 比之前的 js 版本优化多少？加上对比

@fengmk2
Copy link
Member

fengmk2 commented May 11, 2017

ci 加上 node 7

@codecov
Copy link

codecov bot commented May 11, 2017 •
edited
Loading

Codecov Report

Merging #74 into master will decrease coverage by 0.57%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
- Coverage   96.09%   95.51%   -0.58%     
==========================================
  Files           7        7              
  Lines        1076     1093      +17     
  Branches      202      204       +2     
==========================================
+ Hits         1034     1044      +10     
- Misses         42       49       +7
Impacted Files Coverage Δ
index.js 100% <100%> (ø) ⬆️
lib/v1/decoder.js 98.33% <100%> (+0.07%) ⬆️
lib/v2/decoder.js 92.52% <100%> (+0.13%) ⬆️
lib/utils.js 83.33% <0%> (-16.67%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1bea205...f8ad41a. Read the comment docs.

@gxcsoccer
Copy link
Member Author

@pmq20 你跑的时候 byte 还没有合并，现在再跑跑试试

@gxcsoccer
Copy link
Member Author

优化前

  Hessian Decode Benchmark
  node version: v7.10.0, date: Thu May 11 2017 16:53:41 GMT+0800 (CST)
  Starting...
  3 tests completed.

  hessian2 decode: number x 1,647,104 ops/sec ±1.51% (83 runs sampled)
  hessian2 decode: date   x   412,326 ops/sec ±2.43% (81 runs sampled)
  hessian2 decode: string x   169,817 ops/sec ±1.93% (84 runs sampled)

优化后

  Hessian Decode Benchmark
  node version: v7.10.0, date: Thu May 11 2017 16:54:07 GMT+0800 (CST)
  Starting...
  3 tests completed.

  hessian2 decode: number x 9,965,533 ops/sec ±2.19% (85 runs sampled)
  hessian2 decode: date   x 3,342,223 ops/sec ±2.62% (81 runs sampled)
  hessian2 decode: string x 1,272,044 ops/sec ±1.73% (84 runs sampled)

@gxcsoccer
Copy link
Member Author

number 和 string 的优化主要来自于 byte.get node-modules/byte#25

@pmq20
Copy link

pmq20 commented May 11, 2017 

  hessian2 decode: number x 9,840,410 ops/sec ±1.10% (92 runs sampled)
  hessian2 decode: date   x 3,965,427 ops/sec ±0.47% (90 runs sampled)
  hessian2 decode: string x 1,473,963 ops/sec ±0.72% (91 runs sampled)

  Fast hessian2 decode: number x 10,883,935 ops/sec ±1.01% (91 runs sampled)
  Fast hessian2 decode: date   x  4,356,561 ops/sec ±0.88% (90 runs sampled)
  Fast hessian2 decode: string x  1,886,761 ops/sec ±4.78% (62 runs sampled)

@pmq20
Copy link

pmq20 commented May 11, 2017

💯 确实性能和 fast hessian2 差不多了

@fengmk2
Copy link
Member

fengmk2 commented Aug 2, 2017

@gxcsoccer 这周忙完继续搞这个，最近 rpc 比较多压力。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dead-horse dead-horse dead-horse left review comments

@fengmk2 fengmk2 fengmk2 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gxcsoccer @mention-bot @pmq20 @fengmk2 @dead-horse