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

fix: avoid big parent in sliced string #7

Merged
merged 1 commit into from
Oct 29, 2018
Merged

Conversation

gxcsoccer
Copy link
Member

@gxcsoccer gxcsoccer commented Oct 29, 2018

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

#6

@gxcsoccer
Copy link
Member Author

avoid sliced string 有效的几种方式

  • new Buffer(original_string).toString()
    image

  • original_string.split('').join('')
    image

  • (' ' + original_string).slice(1)
    image

会引入一个新的 slice string 但是体积只比需要的 string 多一个空格
image

原来的 benchmark 结果
image

前两种对于性能影响较大,基本上慢了 50%,最后一种会引入一个新的 slice string 但是体积只比要的 string 大一,性能也能接受,所以权衡下来采用第三种

另外 flatstr 这个方式,我试过不能消除 slice string

@gxcsoccer
Copy link
Member Author

window 单测还没有 node 11 的,所以暂时忽略

@fengmk2
Copy link
Member

fengmk2 commented Oct 29, 2018

能否加一个可以引起内存泄漏的测试?就是不修改现在的代码,跑这个测试,会出现内存 oom 。

@gxcsoccer
Copy link
Member Author

gxcsoccer commented Oct 29, 2018

能否加一个可以引起内存泄漏的测试?就是不修改现在的代码,跑这个测试,会出现内存 oom 。

这个我试了下,比较难模拟,我理解这个其实不是泄露,而是多占用一些内容,另外 gc 亲和性下降。

不过有一个 demo http://blog.varunajayasiri.com/string_slice.html ,他证明如果不回收资源,sliced string 会比没有 sliced string 先崩溃

// https://github.com/nodejs/help/issues/711
// https://stackoverflow.com/questions/31712808/how-to-force-javascript-to-deep-copy-a-string
const r = str.replace(REG_STR_REPLACER, a => DECODER_REPLACER[a]);
return (' ' + r).slice(1);
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

Choose a reason for hiding this comment

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

或者 String(r) 呢?

Copy link
Member Author

@gxcsoccer gxcsoccer Oct 29, 2018

Choose a reason for hiding this comment

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

都没效果,我试过,它居然会判断是否发生变化

Copy link
Member

@fengmk2 fengmk2 Oct 29, 2018

Choose a reason for hiding this comment

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

JSON.parse(JSON.stringify(r)) 也加上看看是否性能最差的。

@codecov
Copy link

codecov bot commented Oct 29, 2018

Codecov Report

Merging #7 into master will increase coverage by 0.01%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #7      +/-   ##
=========================================
+ Coverage   97.08%   97.1%   +0.01%     
=========================================
  Files           3       3              
  Lines         206     207       +1     
=========================================
+ Hits          200     201       +1     
  Misses          6       6
Impacted Files Coverage Δ
lib/encoder.js 100% <100%> (ø) ⬆️
lib/decoder.js 94.44% <75%> (+0.05%) ⬆️

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 25184dd...ba0453b. Read the comment docs.

@fengmk2
Copy link
Member

fengmk2 commented Oct 29, 2018

@gxcsoccer 那现在我们如何判断已经修复了呢?

@gxcsoccer
Copy link
Member Author

@gxcsoccer 那现在我们如何判断已经修复了呢?

dump heap 然后看 parent 是否还是原始的

@fengmk2
Copy link
Member

fengmk2 commented Oct 29, 2018

另外 flatstr 这个方式,我试过不能消除 slice string

flatstr 无效的话,得去给它提个 issue 或者 pr,不能让它继续误导使用者。

@gxcsoccer
Copy link
Member Author

@fengmk2 flatstr 他主要不是解决 sliced string 的问题,因为我用 heapdump 是在 node 6 下做的,所以我怀疑 Number(s) 这个黑科技不能解决 sliced string,但是 %FlattenString(s) 这个 api 可能是能解决的

@gxcsoccer
Copy link
Member Author

@fengmk2 flatstr 他主要不是解决 sliced string 的问题,因为我用 heapdump 是在 node 6 下做的,所以我怀疑 Number(s) 这个黑科技不能解决 sliced string,但是 %FlattenString(s) 这个 api 可能是能解决的

https://github.com/davidmarkclements/flatstr/blob/master/index.js

@fengmk2 fengmk2 merged commit 52d05f7 into master Oct 29, 2018
@fengmk2 fengmk2 deleted the fix-sliced-string branch October 29, 2018 08:34
@fengmk2
Copy link
Member

fengmk2 commented Oct 29, 2018

@gxcsoccer 你来发

@gxcsoccer
Copy link
Member Author

1.0.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants