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: 27: ReadableStreamingData should implement bulk read methods #129

Merged
merged 3 commits into from
Dec 4, 2023

Conversation

artemananiev
Copy link
Member

Fix summary:

  • readBytes() methods are implemented in ReadableStreamingData in a more efficient way than inherited from ReadableSequentialData
  • Unit tests are refactored, so some tests previously run for ReadableStreamingData only now are also used for other implementations of ReadableSequentialData (e.g. BufferedData)

Fixes: #27
Signed-off-by: Artem Ananev artem.ananev@swirldslabs.com

Signed-off-by: Artem Ananev <artem.ananev@swirldslabs.com>
@artemananiev artemananiev added the Performance Issues related to performance concerns. label Dec 1, 2023
@artemananiev artemananiev added this to the 0.7.8 milestone Dec 1, 2023
@artemananiev artemananiev self-assigned this Dec 1, 2023
Copy link

github-actions bot commented Dec 1, 2023

JUnit Test Report

     45 files  +  2       45 suites  +2   3m 30s ⏱️ -36s
   737 tests +24     736 ✔️ +24    1 💤 ±0  0 ±0 
5 787 runs  +53  5 769 ✔️ +53  18 💤 ±0  0 ±0 

Results for commit 8d3754f. ± Comparison against base commit 4aa8a08.

This pull request removes 3 and adds 26 tests. Note that renamed tests count towards both.
, 1
com.hedera.pbj.runtime.Utf8ToolsTest ‑ [4] 
com.hedera.pbj.runtime.io.stream.ReadableStreamingDataTest ‑ Read past the data - EOF
com.hedera.pbj.runtime.Utf8ToolsTest ‑ [4] 
, 1
com.hedera.pbj.runtime.io.ReadableSequentialDataTest ‑ Limit snaps to position if set to less than position
com.hedera.pbj.runtime.io.ReadableSequentialDataTest ‑ Read past the buffered data - EOF
com.hedera.pbj.runtime.io.ReadableSequentialDataTest ‑ Read past the byte array - EOF
com.hedera.pbj.runtime.io.ReadableSequentialDataTest ‑ Read some bytes
com.hedera.pbj.runtime.io.ReadableSequentialDataTest ‑ Read some bytes, skip some bytes, read some more bytes
com.hedera.pbj.runtime.io.ReadableSequentialDataTest ‑ Read up to some limit and then extend the limit
com.hedera.pbj.runtime.io.ReadableSequentialDataTest ‑ Read up to some limit on the stream which is less than its total length
com.hedera.pbj.runtime.io.ReadableSequentialDataTest ‑ Skip some bytes, read some bytes
com.hedera.pbj.runtime.io.ReadableSequentialDataTest ‑ Skipping more bytes than are available
…

♻️ This comment has been updated with latest results.

Copy link
Contributor

codacy-production bot commented Dec 1, 2023

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.68% (target: -1.00%) 89.87%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (4aa8a08) 3977 1232 30.98%
Head commit (8d3754f) 4037 (+60) 1278 (+46) 31.66% (+0.68%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#129) 79 71 89.87%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Copy link

github-actions bot commented Dec 1, 2023

Integration Test Report

86 963 tests  ±0   86 963 ✔️ ±0   5m 50s ⏱️ + 1m 22s
     213 suites ±0            0 💤 ±0 
     213 files   ±0            0 ±0 

Results for commit 8d3754f. ± Comparison against base commit 4aa8a08.

♻️ This comment has been updated with latest results.

Copy link
Member

@jsync-swirlds jsync-swirlds left a comment

Choose a reason for hiding this comment

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

I believe we could simplify much of this with a slightly different approach.
Interested in your take on this.

Addressed reviewers' comments
Implemented one more method in WriteableStreamingData
Removed some unused imports

Signed-off-by: Artem Ananev <artem.ananev@swirldslabs.com>
Fixed WriteableStreamingData.writeBytes(ByteBuffer) and added a new unit test for it

Signed-off-by: Artem Ananev <artem.ananev@swirldslabs.com>
Copy link
Contributor

@anthony-swirldslabs anthony-swirldslabs left a comment

Choose a reason for hiding this comment

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

Looks good.

@artemananiev artemananiev merged commit 4823362 into main Dec 4, 2023
10 checks passed
@artemananiev artemananiev deleted the 00027-M-readablestreamingdata-bulk-reads branch December 4, 2023 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance Issues related to performance concerns.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReadableStreamingData should implement bulk read methods
3 participants