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

Unsure to merge: Compress blocks and txs as well #761

Closed
wants to merge 2 commits into from
Closed

Conversation

dahlia
Copy link
Contributor

@dahlia dahlia commented Jan 15, 2020

This patch is continued from #753. Besides block states, this compresses blocks and txs as well. Unfortunately, the compression rate is almost the same to #753 (5.1G → 1.6G).

@dahlia dahlia added discussion needed We need to dicuss about this storage Related to storage (Libplanet.Store) labels Jan 15, 2020
@dahlia dahlia self-assigned this Jan 15, 2020
@codecov
Copy link

codecov bot commented Jan 15, 2020

Codecov Report

Merging #761 into master will increase coverage by 0.08%.
The diff coverage is 92.18%.

@@            Coverage Diff             @@
##           master     #761      +/-   ##
==========================================
+ Coverage    86.4%   86.48%   +0.08%     
==========================================
  Files         222      223       +1     
  Lines       19218    19273      +55     
==========================================
+ Hits        16605    16669      +64     
+ Misses       1419     1410       -9     
  Partials     1194     1194
Impacted Files Coverage Δ
Libplanet.Tests/Store/DefaultStoreFixture.cs 96.29% <100%> (+0.84%) ⬆️
...ibplanet.Tests/Store/CompressedDefaultStoreTest.cs 100% <100%> (ø)
Libplanet.Tests/Store/DefaultStoreTest.cs 98.07% <100%> (ø) ⬆️
Libplanet/Store/DefaultStore.cs 85.63% <90.38%> (+2.79%) ⬆️
Libplanet.Tests/Net/Protocols/TestTransport.cs 75.57% <0%> (-0.66%) ⬇️

@longfin
Copy link
Member

longfin commented Jan 15, 2020

I guess it's okay that we contain this feature and can also drop out when it is obvious that it is not necessary or harmful to features.

deflate.Flush();
buffer.Flush();
int length = (int)buffer.Position;
var output = new byte[length];
Copy link
Member

Choose a reason for hiding this comment

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

Can we use buffer.ToArray() instead of temp variable(output)?

deflate.Flush();
outputBuffer.Flush();
int length = (int)outputBuffer.Position;
var output = new byte[length];
Copy link
Member

Choose a reason for hiding this comment

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

Can we use outputBuffer.ToArray() instead of temp variable(output)?

@dahlia
Copy link
Contributor Author

dahlia commented Jan 23, 2020

As this patch does not solve the problem and is likely to make the code more complicated, I decided to do not merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion needed We need to dicuss about this storage Related to storage (Libplanet.Store)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants