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(virtual-fs): Fix horrible write performance for in-memory files #3905

Closed
wants to merge 1 commit into from

Conversation

theduke
Copy link
Contributor

@theduke theduke commented May 24, 2023

The write() implementation for in-memory files used by the TmpFs /
mem_fs was terrible: it used to re-allocate and copy the whole data if
the write position was not at the end of the file.

This re-writes the implementation to grow the buffer in place if
required.

Also adds some tests.

Closes #3903

@theduke theduke requested a review from ptitSeb May 24, 2023 12:35
@theduke theduke force-pushed the issue-3903-fix-slow-memfs-write branch from 310ae50 to 4def192 Compare May 24, 2023 12:38
The write() implementation for in-memory files used by the TmpFs /
mem_fs was terrible: it used to re-allocate and copy the whole data if
the write position was not at the end of the file.

This re-writes the implementation to grow the buffer in place if
required.

Also adds some tests.

Closes #3903
@theduke theduke force-pushed the issue-3903-fix-slow-memfs-write branch from 4def192 to 047823e Compare May 24, 2023 12:39

self.buffer = new_buffer;
}
if buf.is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't the file by truncated in that case, depending on the write mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The buf is only the newly written data.

self.buffer.append(&mut remainder)?;
}
if end_cursor > self.buffer.len() {
self.buffer.resize(end_cursor, 0)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't the file (well the vec) be srinked if it's smaller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why it's inside an if check.
This only runs when the new data makes the file larger than it previously was.


file.write(b" mkay?", &mut cursor).unwrap();
assert_eq!(file.buffer.deref(), b"alter the universe! mkay?");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a test with a seek and truncate could be interesting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely.

We should have a proper test suite for trait FileSystem implementations.

I only added a tests for write() because that's the only thing I changed.

@theduke
Copy link
Contributor Author

theduke commented Aug 21, 2023

Superseded by #4165

@theduke theduke closed this Aug 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.

Fix Incredibly Slow write Implementation of TmpFs In-Memory Files
2 participants