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

add: recycle buffer #704

Merged
merged 2 commits into from
Apr 1, 2019
Merged

Conversation

CoffeeLatte007
Copy link
Contributor

Ⅰ. Describe what this PR did

In the code, there are many bytebuffers created but not reused. To reduce the number of YGC, ByteBuffer is reused.

I first read netty's recycle and implemented it. But this is a big change and will make the byte[] that was previously returned by SessionStorable into one of our custom objects, so I dropped it.

Then I looked at other store's system, such as RocketMq, which use ThreadLocal for caching,.it is simple and has very little momentum.

I ran the write part of WriteStoreTest, the VM parameter was -Xmx10m, the YGC was 3,400 times before the modification, and the YGC was 900 times after the modification.

I also changed the HeapByteBuffer used by Filechannel to DirectHeapByteBuffer.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

run write part of WriteStoreTest, VM parameter -Xmx10m
wathching jstat -gc pid 1000

Ⅴ. Special notes for reviews

@codecov-io
Copy link

codecov-io commented Mar 31, 2019

Codecov Report

Merging #704 into develop will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop     #704      +/-   ##
=============================================
+ Coverage      32.71%   32.72%   +0.01%     
- Complexity       895      896       +1     
=============================================
  Files            227      226       -1     
  Lines           8804     8797       -7     
  Branches        1061     1058       -3     
=============================================
- Hits            2880     2879       -1     
+ Misses          5598     5595       -3     
+ Partials         326      323       -3
Impacted Files Coverage Δ Complexity Δ
...m/alibaba/fescar/server/session/BranchSession.java 89.28% <100%> (+0.19%) 31 <4> (+1) ⬆️
...scar/server/store/FileTransactionStoreManager.java 53.15% <100%> (+0.24%) 15 <0> (ø) ⬇️
...m/alibaba/fescar/server/session/GlobalSession.java 85.81% <100%> (+0.19%) 42 <1> (+1) ⬆️
.../fescar/server/session/AbstractSessionManager.java 74.07% <0%> (ø) 19% <0%> (ø) ⬇️
...fescar/server/session/FileBasedSessionManager.java 54.87% <0%> (ø) 15% <0%> (ø) ⬇️
...ibaba/fescar/rm/datasource/undo/BranchUndoLog.java 100% <0%> (ø) 7% <0%> (ø) ⬇️
...a/com/alibaba/fescar/core/protocol/ResultCode.java 75% <0%> (ø) 4% <0%> (ø) ⬇️
... and 114 more

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 d049f3e...1a21f74. Read the comment docs.

Copy link
Contributor

@github-ygy github-ygy left a comment

Choose a reason for hiding this comment

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

i left some comments

Copy link
Contributor

@jovany-wang jovany-wang left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM

Copy link
Contributor

@zhangthen zhangthen left a comment

Choose a reason for hiding this comment

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

+1

@zhangthen zhangthen merged commit 5be45d7 into apache:develop Apr 1, 2019
@@ -299,8 +303,10 @@ public void run() {

private boolean writeDataFile(byte[] bs) {
int retry = 0;
byte[] byWrite = new byte[bs.length + 4];
ByteBuffer byteBuffer = ByteBuffer.wrap(byWrite);
ByteBuffer byteBuffer = wireteBuffer;
Copy link
Member

Choose a reason for hiding this comment

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

if size > 4096?

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

Successfully merging this pull request may close these issues.

None yet

7 participants