Add a size hint attribute for fsreplace1
#20394
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Vulture doesn't detect this because of the `_`. There doesn't seem to be a way to ignore locals prefixed with `_` while still catching such instance variables.
I was so young and naive when I wrote these...
We call `asyncio.get_running_loop()` on every single `write()`. Let's cache the attribute on the channel from the start. This will let us avoid calling `.get_running_loop()` from some other users as well, in the next commit.
There are a fair number of uses of .run_in_executor() sprinkled around AsyncChannel implementations. Let's add a trivial helper for those. Getting the types right for this in a way that was backwards compatible with Python 3.6 was fairly difficult, since 'typing.ParamSpec' doesn't even exist there.
If we could depend on Python 3.12 and use delete_on_close=False then using NamedTemporaryFile as a context manager would be really useful. Since we can't, and already have to deal with the deletion manually, let's also just deal with the close part manually. Getting an fd and a filename fits our uses a lot better anyway: most things we call want to operate on an fd and having the name in a variable lets us easily flag the cases where we don't want to try to delete it. We move the `finally:` handling of closing the fd and (conditionally) removing the temporary file to a separate internal `try:` block. This has nothing to do with the delete case (which does want to share the error handling, which is now the only thing the outer `try:` does).
The entire `fsreplace1` channel is implemented in a single function, and it's getting a bit complicated (and is about to get even more complicated). Split things up a bit.
allisonkarlitskaya
requested review from
martinpitt and
jelly
and removed request for
martinpitt
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Why can't we have nice things? |
allisonkarlitskaya
force-pushed
the
fsreplace-size-hint
branch
from
b2e585e
to
1fdcca7
Compare
17 tasks
martinpitt
requested changes
Apr 29, 2024
| if size is not None: | ||
| logger.debug('fallocate(%s.tmp, %d)', path, size) | ||
| if size: # posix_fallocate() of 0 bytes is EINVAL | ||
| await self.in_thread(os.posix_fallocate, fd, 0, size) |
There was a problem hiding this comment.
While at it, does it also make sense to add posix_fadvise(os.POSIX_FADV_DONTNEED) to avoid cache trashing? (This can be a follow-up of course)
There was a problem hiding this comment.
I'm not sure we know that as a general rule — it may well be the case that the user wants to use this immediately after uploading it.
This calls posix_fallocate() before starting to write the data to the disk. This has two benefits: - allocating the data in one go is better for fragmentation. Without a size hint given in advance, the kernel has no idea how big of a block it needs. With a slow upload, the kernel will start committing data to disk long before it knows the total size. - allocating the data at the start means that we can detect "out of space" early, before we waste time uploading 90% of a huge file
Instead of taking a ReadableStream, take a Blob. The reason for this is to gain access to the `.size` attribute on the Blob, allowing us to send a `size` hint on the `fsreplace1` channel. I considered giving the possibility of accepting either, but this API is totally new (only appearing in the last release), there are no known existing users, and is bundled, anyway.
allisonkarlitskaya
force-pushed
the
fsreplace-size-hint
branch
from
1fdcca7
to
0fcaef1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Plus some associated cleanups.
Not in this PR: sending the
sizeattribute fromcockpit.file().replace()— the way we handle iterating over string data (which may contain non-ascii characters) makes this quite difficult at present.