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

buffer read and write #1581

Merged
merged 1 commit into from
Feb 21, 2023
Merged

buffer read and write #1581

merged 1 commit into from
Feb 21, 2023

Conversation

wlsnx
Copy link
Contributor

@wlsnx wlsnx commented Feb 17, 2023

https://github.com/serde-rs/json/blob/0ebeede28a0d5f0f07f18124078f55d62b180a2d/src/ser.rs#L1540
https://github.com/serde-rs/json/blob/0ebeede28a0d5f0f07f18124078f55d62b180a2d/src/read.rs#L194

before:
write(3, "{", 1) = 1
write(3, "\n", 1) = 1
write(3, " ", 2) = 2
write(3, """, 1) = 1
write(3, "ociVersion", 10) = 10
write(3, """, 1) = 1
write(3, ": ", 2) = 2
write(3, """, 1) = 1
write(3, "1.0.2-dev", 9) = 9
write(3, """, 1) = 1
...

read(3, "{", 1) = 1
read(3, "\n", 1) = 1
read(3, " ", 1) = 1
read(3, " ", 1) = 1
read(3, """, 1) = 1
read(3, "o", 1) = 1
read(3, "c", 1) = 1
read(3, "i", 1) = 1
read(3, "V", 1) = 1
read(3, "e", 1) = 1
read(3, "r", 1) = 1
read(3, "s", 1) = 1
...

after:
read(3, "{\n "ociVersion": "1.0.2-dev",\n "..., 8192) = 3293
write(3, "{\n "ociVersion": "1.0.2-dev",\n "..., 3282) = 3282

@codecov-commenter
Copy link

codecov-commenter commented Feb 19, 2023

Codecov Report

Merging #1581 (03f60ba) into main (8b0c44b) will decrease coverage by 0.02%.
The diff coverage is 57.14%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1581      +/-   ##
==========================================
- Coverage   68.86%   68.85%   -0.02%     
==========================================
  Files         120      120              
  Lines       13058    13072      +14     
==========================================
+ Hits         8993     9001       +8     
- Misses       4065     4071       +6     

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Feb 20, 2023

Hey, if I understand correctly, this changes normal read/write to buffered, so that number of syscalls are reduced?

@wlsnx
Copy link
Contributor Author

wlsnx commented Feb 20, 2023

Hey, if I understand correctly, this changes normal read/write to buffered, so that number of syscalls are reduced?

Yes,serde_json read content from reader byte by byte,if without buffered,there are huge number of read syscalls.Similarly,serde_json will call write for every element.

Comment on lines 113 to 115
let file = File::create(path)?;
let writer = BufWriter::new(file);
to_writer_pretty(writer, &spec)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey, I know this is a test, but should we not also add a writer.flush() at the end, like we have done at other places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://doc.rust-lang.org/nightly/std/io/struct.BufWriter.html
It is critical to call flush before BufWriter is dropped. Though dropping will attempt to flush the contents of the buffer, any errors that happen in the process of dropping will be ignored. Calling flush ensures that the buffer is empty and thus dropping will not even attempt file operations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, so we should add a flush call after to_writer_pretty like other places right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So can you update and push it, so we can go ahead and merge this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Signed-off-by: zhuli <wanlisnx@gmail.com>
Copy link
Collaborator

@YJDoc2 YJDoc2 left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Feb 21, 2023

Thanks a lot for making this improvement !

@YJDoc2 YJDoc2 merged commit a2ec94c into youki-dev:main Feb 21, 2023
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.

3 participants