Read clarification #23847
Read clarification #23847
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pcwalton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see CONTRIBUTING.md for more information. |
ghost
mentioned this pull request
|
The call to reserve here is actually totally redundant and can be removed without changing the operation in any way - This does actually have a very large functional change. Unless the input has a size perfectly on a power of 2 boundary, there's going to be some amount of buffer that we have to truncate at the end of this method. The amount of memory that's zeroed between calls to read now grows without bound. If I am reading a 64 MB + 1 byte file, the buffer will need to be resized for the last time after the first 64 MB of the file has been read. However much we extend the buffer by here, we're only going to be adding one byte before reaching EOF. In the current implementation, we'll zero 64KB, so have wasted 64KB - 1 byte of zeroing work. On the other hand, this PR will bump the wasted work to 64MB - 1 byte. In general, the amount of wasted work is going to scale with the size of the file which seems not great. |
|
I think you're right, you could completely remove reserve() here with no problem. I can take a look.
It was growing without bound before as well. Since reserve() always rounds up to the nearest power of 2. After you reallocate with 16, you are rounded up to a power of two- then you double, then you double, etc, since you keep rounding up to the next power of two. For example- let's say we start with 1000 bytes. In the code before this PR, it requests 16 bytes, and is rounded up to 1024 bytes total. Then it requests 32 and is rounded up to 2048. Then it requests 64 and is rounded up to 5096. etc.
If you only consider the last call to reserve/realloc what you are saying is true. But there are a lot of reallocs before the last realloc, and a WHOLE lot more for the case where we increase by a constant than if we double every time. Perhaps my explanation with a geometric series/triangular numbers was convoluted, Dr. Dobbs has an article about it http://www.drdobbs.com/c-made-easier-how-vectors-grow/184401375. The doubling pattern is O(n) whereas the increase by a constant is O(n^2). Ie increasing by a constant scales FAR worse with the size of the file. |
|
You are correct though, that my version (as well as the current version) zero unnecessarily large amounts of data. And that the call to reserve() is unnecessary, since it is being called in extend(). I've got a fix I'm testing, I'll commit and push it after it compiles and the tests run. |
reallocation strategy since extend() calls reserve() and/or push() for us.
|
EDIT: actually, the new implementation looks good to me, sorry. I should probably have looked at the commit before commenting :) |
|
@sfackler Can you talk on #rust-internals for a minute? |
|
⌛ Testing commit 240734c with merge c4dc2fd... |
|
⛄ The build was interrupted to prioritize another pull request. |
|
⌛ Testing commit 240734c with merge 92d0785... |
…kler This introduces no functional changes except for reducing a few unnecessary operations and variables. Vec has the behavior that, if you request space past the capacity with reserve(), it will round up to the nearest power of 2. What that effectively means is that after the first call to reserve(16), we are doubling our capacity every time. So using the DEFAULT_BUF_SIZE and doubling cap_size() here is meaningless and has no effect on the call to reserve(). Note that with rust-lang#23842 implemented this will hopefully have a clearer API and less of a need for commenting. If rust-lang#23842 is not implemented then the most clear implementation would be to call reserve_exact(buf.capacity()) at every step (and making sure that buf.capacity() is not zero at the beginning of the function of course). Edit- functional change now introduced. We will now zero 16 bytes of the vector first, then double to 32, then 64, etc. until we read 64kB. This stops us from zeroing the entire vector when we double it, some of which may be wasted work. Reallocation still follows the doubling strategy, but the responsibility has been moved to vec.extend(), which calls reserve() and push_back().
|
⛄ The build was interrupted to prioritize another pull request. |
|
⚡ Previous build results for auto-linux-64-nopt-t are reusable. Rebuilding only auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt... |
|
⛄ The build was interrupted to prioritize another pull request. |
|
⚡ Previous build results for auto-linux-64-nopt-t, auto-mac-64-opt are reusable. Rebuilding only auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt... |
|
💔 Test failed - auto-win-32-nopt-t |
|
@bors: retry On Wed, Apr 1, 2015 at 5:20 PM, bors notifications@github.com wrote:
|
|
@bors: rollup |
This introduces no functional changes except for reducing a few unnecessary operations and variables. Vec has the behavior that, if you request space past the capacity with reserve(), it will round up to the nearest power of 2. What that effectively means is that after the first call to reserve(16), we are doubling our capacity every time. So using the DEFAULT_BUF_SIZE and doubling cap_size() here is meaningless and has no effect on the call to reserve().
Note that with #23842 implemented this will hopefully have a clearer API and less of a need for commenting. If #23842 is not implemented then the most clear implementation would be to call reserve_exact(buf.capacity()) at every step (and making sure that buf.capacity() is not zero at the beginning of the function of course).
Edit- functional change now introduced. We will now zero 16 bytes of the vector first, then double to 32, then 64, etc. until we read 64kB. This stops us from zeroing the entire vector when we double it, some of which may be wasted work. Reallocation still follows the doubling strategy, but the responsibility has been moved to vec.extend(), which calls reserve() and push_back().