WIP: Add RawVec to unify raw Vecish code #26955

Gankra · 2015-07-10T23:33:20Z

Per the top level comment:

A low-level utility for more ergonomically allocating, reallocating, and deallocating a
a buffer of memory on the heap without having to worry about all the corner cases
involved. This type is excellent for building your own data structures like Vec and VecDeque.
In particular:

Produces heap::EMPTY on zero-sized types
Produces heap::EMPTY on zero-length allocations
Catches all overflows in capacity computations (promotes them to "capacity overflow" panics)
Guards against 32-bit systems allocating more than isize::MAX bytes
Guards against overflowing your length
Aborts on OOM
Avoids freeing heap::EMPTY
Contains a ptr::Unique and thus endows the user with all related benefits

This type does not in anyway inspect the memory that it manages. When dropped it will
free its memory, but it won't try to Drop its contents. It is up to the user of RawVec
to handle the actual things stored inside of a RawVec.

Note that a RawVec always forces its capacity to be usize::MAX for zero-sized types.
This enables you to use capacity growing logic catch the overflows in your length
that might occur with zero-sized types.

However this means that you need to be careful when roundtripping this type
with a Box<[T]>: cap() won't yield the len. However with_capacity,
shrink_to_fit, and from_box will actually set RawVec's private capacity
field. This allows zero-sized types to not be special-cased by consumers of
this type.

Edit:
fixes #18726 and fixes #23842

Gankra · 2015-07-10T23:33:29Z

CC @rust-lang/libs

rust-highfive · 2015-07-10T23:33:32Z

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

Gankra · 2015-07-10T23:36:57Z

src/libcollections/string.rs

-#[deprecated(since = "1.2.0",
-             reason = "replaced with deref coercions or Borrow")]
-#[allow(deprecated)]
-pub struct DerefString<'a> {


Note: I deleted this stuff since it's been deprecated for a while and porting it to the new stuff would be more effort than it's worth.

reem · 2015-07-10T23:38:59Z

cc me

reem · 2015-07-10T23:47:31Z

src/liballoc/raw_vec.rs

+// this is still enough for space for what the user requested, because otherwise
+// the computation of `2 * needed_size` would have overflowed `usize::MAX`.
+
+/*


Appears to be dead code.

Yeah I should probably just remove this...

Gankra · 2015-07-11T03:39:10Z

Bench results:
https://gist.github.com/Gankro/4eed4b4842045114333f

Vec seems to be uniformly like 10% slower maybe? Our benches are so noisy...

Gankra · 2015-07-11T03:53:56Z

src/libcollections/vec.rs

-            if old_size >= MAX_MEMORY_SIZE { panic!("capacity overflow") }
-            let mut size = max(old_size, 2 * mem::size_of::<T>()) * 2;
-            if old_size > size || size > MAX_MEMORY_SIZE {
-                size = MAX_MEMORY_SIZE;


Upon reflection, this is just wrong... MAX_MEMORY_SIZE is in no way a multiple of size_of<T>? Also the capacity isn't updated...? Also this seems to be relying on * 2 wrapping?

This assumes that mem::size_of::<T>() <= (~0)/4 and MAX_MEMORY_SIZE-1 <= (~0)/2

@arielb1 You still can't just set size to MAX_MEMORY_SIZE, right?

Gankra · 2015-07-11T05:20:19Z

Added some extra micro-optimization in the same vein as Vec used to have. (updated the gist)

Now it looks like it's 10% faster than the old impl... but it looks like everything looked about that much faster this time (even unrelated stuff).

Benches. How do they work?

Still, this is fixing some soundness issues with the current implementation, and now it looks like it's basically a performance wash?

bluss · 2015-07-11T08:26:15Z

Even unrelated stuff? Cpu frequency scaling often ruins it for me. Hm I thought this looked so good.

But you're right, if I look at for example the LinkedList iteration benchmark as control, then it looks like the benchmark runs happened at different cpu speeds basically.

bluss · 2015-07-11T08:50:14Z

src/libcollections/vec.rs

-                dealloc(self.allocation, self.cap);
-            }
-        }
+        for _x in self.by_ref() {}


I think this compiles nowadays if you just remove the .by_ref()?

petrochenkov · 2015-07-11T11:55:28Z

@gankro
I think all vectors, including zero-sized, should have maximum capacity of isize::MAX... just in case.
(Something like this is what I have in mind.)
Nobody will use huge vectors of zero-sized types anyway.

Gankra · 2015-07-11T21:54:40Z

@petrochenkov we definitely support them today; this seems like a needless regression for a hypothetical future change. If that future change does in fact occur we could consider making the change then.

petrochenkov · 2015-07-11T22:21:21Z

It's easier to make things stricter now, than it will be in the future. Besides, this restriction gives some potentially useful guarantees, for example, index difference never overflows. The restriction could be easily relaxed in the future if a single motivational example for large zero-sized vectors is found.
Anyway, this is not a big deal.

Gankra · 2015-07-11T23:31:52Z

Oh I forgot to mention, this finally fixes #18726 (assuming someone fixed arrays).

Gankra · 2015-07-12T04:33:49Z

@petrochenkov I've never seen "blind" index differences (where you want a signed result) before. What's the usecase look like?

bors · 2015-07-12T11:46:12Z

☔ The latest upstream changes (presumably #26985) made this pull request unmergeable. Please resolve the merge conflicts.

apasel422 · 2015-07-13T16:53:25Z

src/liballoc/raw_vec.rs

+        }
+    }
+
+    /// Creates a RawVec with exactly the capacity and alignment requirements for


Duplicate "for" in this sentence.

bors · 2015-07-14T05:32:29Z

☔ The latest upstream changes (presumably #27002) made this pull request unmergeable. Please resolve the merge conflicts.

Manishearth · 2015-07-16T11:07:29Z

This breaks the debuginfo tests, I think the pretty printers need to be updated so that they can understand the new Vec/String.

cc @michaelwoerister

Gankra · 2015-07-16T18:29:42Z

@bors r-

Oh shoot! I forgot that that was broken! I asked on IRC about that, didn't get a response, and then got distracted and forgot.

Gankra · 2015-07-17T17:58:19Z

I've fixed the pretty printer; need someone to review that change.

alexcrichton · 2015-07-17T18:23:38Z

pretty printer changes look fine to me

Gankra · 2015-07-17T18:27:26Z

@bors r=bluss,alexcrichton

bors · 2015-07-17T18:27:26Z

📌 Commit b0ee1eb has been approved by bluss,alexcrichton

bors · 2015-07-17T20:51:25Z

⌛ Testing commit b0ee1eb with merge de860d5...

alexcrichton · 2015-07-17T20:57:32Z

@bors: retry force

bors · 2015-07-17T20:57:35Z

⌛ Testing commit b0ee1eb with merge af1f70a...

bors · 2015-07-17T22:27:36Z

💔 Test failed - auto-mac-32-opt

Gankra · 2015-07-17T23:12:39Z

@bors retry

Seems spurious...?

Per the top level comment: A low-level utility for more ergonomically allocating, reallocating, and deallocating a a buffer of memory on the heap without having to worry about all the corner cases involved. This type is excellent for building your own data structures like Vec and VecDeque. In particular: * Produces heap::EMPTY on zero-sized types * Produces heap::EMPTY on zero-length allocations * Catches all overflows in capacity computations (promotes them to "capacity overflow" panics) * Guards against 32-bit systems allocating more than isize::MAX bytes * Guards against overflowing your length * Aborts on OOM * Avoids freeing heap::EMPTY * Contains a ptr::Unique and thus endows the user with all related benefits This type does not in anyway inspect the memory that it manages. When dropped it *will* free its memory, but it *won't* try to Drop its contents. It is up to the user of RawVec to handle the actual things *stored* inside of a RawVec. Note that a RawVec always forces its capacity to be usize::MAX for zero-sized types. This enables you to use capacity growing logic catch the overflows in your length that might occur with zero-sized types. However this means that you need to be careful when roundtripping this type with a `Box<[T]>`: `cap()` won't yield the len. However `with_capacity`, `shrink_to_fit`, and `from_box` will actually set RawVec's private capacity field. This allows zero-sized types to not be special-cased by consumers of this type. Edit: fixes #18726 and fixes #23842

bors · 2015-07-17T23:58:53Z

⌛ Testing commit b0ee1eb with merge e58601a...

bors · 2015-07-18T01:06:08Z

💔 Test failed - auto-mac-64-opt

Gankra · 2015-07-18T01:06:57Z

@bors retry

bors · 2015-07-18T01:10:50Z

⚡ Previous build results for auto-mac-32-opt, auto-mac-64-nopt-t are reusable. Rebuilding only auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt...

bors · 2015-07-18T03:42:00Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, 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-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt

Gankra · 2015-07-18T03:44:04Z

yusssss

nox · 2020-09-30T10:43:15Z

This probably doesn't matter in real life, but I just realised this PR regressed Cow::deref because prior to this PR, both variants of Cow<str> had (ptr, len) in that order, meaning Cow::deref just returned that pair in both paths.

rust-highfive assigned huonw Jul 10, 2015

Gankra reviewed Jul 10, 2015
View reviewed changes

reem reviewed Jul 10, 2015
View reviewed changes

Gankra reviewed Jul 11, 2015
View reviewed changes

bluss reviewed Jul 11, 2015
View reviewed changes

Gankra force-pushed the raw-vec branch from b981b61 to cf36380 Compare July 12, 2015 16:59

apasel422 reviewed Jul 13, 2015
View reviewed changes

Gankra force-pushed the raw-vec branch 4 times, most recently from 48ca988 to d8a862c Compare July 14, 2015 20:14

Add RawVec to unify raw Vecish code

bfa0e1f

Gankra force-pushed the raw-vec branch from 423b8a5 to d97bc2a Compare July 17, 2015 17:41

fix pretty printers to handle new Vec

b0ee1eb

Gankra force-pushed the raw-vec branch from d97bc2a to b0ee1eb Compare July 17, 2015 17:44

Gankra mentioned this pull request Jul 17, 2015

doc: use 'index' and 'value' in place of 'i' and 't' #27100

Merged

bors merged commit b0ee1eb into rust-lang:master Jul 18, 2015

bors mentioned this pull request Jul 18, 2015

desugaring-based box placement-in (take-4 branch) #26180

Closed

Gankra mentioned this pull request Jul 18, 2015

array types (fixed-size arrays on 32-bit, HashMap, Vec) can be large enough that indexing is unsound #18726

Closed

bluss mentioned this pull request Nov 5, 2015

vec: Remove old comment in Vec::drop #29614

Merged

WIP: Add RawVec to unify raw Vecish code #26955

WIP: Add RawVec to unify raw Vecish code #26955

Conversation

Gankra commented Jul 10, 2015

Gankra commented Jul 10, 2015

rust-highfive commented Jul 10, 2015

Choose a reason for hiding this comment

reem commented Jul 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gankra commented Jul 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gankra commented Jul 11, 2015

bluss commented Jul 11, 2015

Choose a reason for hiding this comment

petrochenkov commented Jul 11, 2015

Gankra commented Jul 11, 2015

petrochenkov commented Jul 11, 2015

Gankra commented Jul 11, 2015

Gankra commented Jul 12, 2015

bors commented Jul 12, 2015

Choose a reason for hiding this comment

bors commented Jul 14, 2015

Manishearth commented Jul 16, 2015

Gankra commented Jul 16, 2015

Gankra commented Jul 17, 2015

alexcrichton commented Jul 17, 2015

Gankra commented Jul 17, 2015

bors commented Jul 17, 2015

bors commented Jul 17, 2015

alexcrichton commented Jul 17, 2015

bors commented Jul 17, 2015

bors commented Jul 17, 2015

Gankra commented Jul 17, 2015

bors commented Jul 17, 2015

bors commented Jul 18, 2015

Gankra commented Jul 18, 2015

bors commented Jul 18, 2015

bors commented Jul 18, 2015

Gankra commented Jul 18, 2015

nox commented Sep 30, 2020