First implementation - Githubissues

I got a dejavu about the PR name

@kvark I did nothing and then it happened. #1 is closed and I can't reopen it. I blame aliens from another dimension.

The branch names are identical..

I will make separate docs pass over this and provide a PR, so I won't be looking at documentation and spelling for this initial review.

Reviewed 9 of 9 files at r1. Review status: 2 of 9 files reviewed at latest revision, 2 unresolved discussions.

src/arena.rs, line 54 at r1 (raw file):

        if self.is_unused() {
            let ArenaNode { block, .. } = self;
            owner.free(device, block);

Why not just owner.free(device, self.block)?

src/block.rs, line 161 at r1 (raw file):

    /// Tags form a stack - e.g. LIFO
    pub fn pop_tag(self) -> (Block<B, T>, Y) {
        let Block { .. } = self;

Why is self deconstructed twice ?

Comments from Reviewable

Reviewed 2 of 9 files at r1, 2 of 7 files at r2. Review status: 4 of 9 files reviewed at latest revision, 10 unresolved discussions.

src/arena.rs, line 62 at r2 (raw file):

}

/// Linear allocator for short-living objects

IIRC it's short-lived

src/arena.rs, line 77 at r2 (raw file):

    A: MemoryAllocator<B>,
{
    /// Construct allocator.

This kind of documentation isn't really useful.

src/block.rs, line 39 at r2 (raw file):

{
    /// Construct untagged block from `Memory` and `Range`.
    /// Pointed `Memory` shouldn't be freed until at least one `Block`s of it

Maybe "The memory pointer has to outlive all blocks it is passed to"?

src/combined.rs, line 14 at r2 (raw file):

    /// For short-living objects.
    /// Such as staging buffers.
    ShortLive,

ShortLived

src/lib.rs, line 93 at r2 (raw file):

    /// Try to dispose of this allocator.
    /// It will result in `Err(self)` if is in use.
    /// Allocators usually will panic on drop.

This needs clarification, something like "Allocators have to be disposed, dropping them might result in a panic."

src/lib.rs, line 107 at r2 (raw file):


    /// This allocator will place a tag of this type on the block.
    /// It is intended to use by allocator in `free` method.

No need to end the sentence :)

src/lib.rs, line 146 at r2 (raw file):

    T: From<u8> + Add<Output = T> + Sub<Output = T> + Rem<Output = T> + Eq + Copy,
{
    if offset == 0.into() {

To avoid the branch, you can do (alignment - offset % alignment) & !0x4

src/root.rs, line 14 at r2 (raw file):

    id: MemoryTypeId,
    allocations: usize,
    pd: PhantomData<fn() -> B>,

You can just use PhantomData<B> here.

Comments from Reviewable

Reviewed 7 of 7 files at r2. Review status: all files reviewed at latest revision, 9 unresolved discussions.

src/arena.rs, line 77 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

This kind of documentation isn't really useful.

I'll be doing a docs PR onto this one tonight hopefully.

Comments from Reviewable

The branch names are identical..

I couldn't reopen #1 before I made this doppelgänger

Review status: all files reviewed at latest revision, 10 unresolved discussions.

src/arena.rs, line 54 at r1 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Why not just `owner.free(device, self.block)`?

No reason :smile:

src/arena.rs, line 62 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

IIRC it's short-lived

Done.

src/block.rs, line 161 at r1 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Why is self deconstructed twice ?

Because I can! Anyway fixed.

src/block.rs, line 39 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Maybe "The `memory` pointer has to outlive all blocks it is passed to"?

Pointer doesn't outlive anything. The memory it is pointing to - does.

src/combined.rs, line 14 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

`ShortLived`

Done.

src/lib.rs, line 93 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

This needs clarification, something like "Allocators have to be disposed, dropping them might result in a panic."

Done.

src/lib.rs, line 107 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

No need to end the sentence :)

Could you care to explain?

src/lib.rs, line 146 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

To avoid the branch, you can do `(alignment - offset % alignment) & !0x4`

Are you sure about the formula? !0x4 looks arbitrary here.

(alignment - offset % alignment) % alignment may work though. But unless compiler can optimize it away it is two divisions instead of one.

src/root.rs, line 14 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

You can just use `PhantomData` here.

Probably I can even not have it here in the first place.

Comments from Reviewable

Reviewed 8 of 8 files at r3. Review status: all files reviewed at latest revision, 8 unresolved discussions.

src/lib.rs, line 107 at r2 (raw file):

Previously, omni-viral (Zakarum) wrote…

Could you care to explain?

I got it.

Comments from Reviewable

Reviewed 2 of 2 files at r4. Review status: all files reviewed at latest revision, 8 unresolved discussions.

Comments from Reviewable

Reviewed 3 of 8 files at r3, 1 of 2 files at r4. Review status: all files reviewed at latest revision, 3 unresolved discussions.

src/block.rs, line 39 at r2 (raw file):

Previously, omni-viral (Zakarum) wrote…

Pointer doesn't outlive anything. The `memory` it is pointing to - does.

That's what I mean when talking about the lifetime of a pointer.

Comments from Reviewable

Reviewed 1 of 3 files at r5, 1 of 11 files at r6. Review status: 2 of 12 files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

Reviewed 10 of 11 files at r6. Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

One more (hopefully, last?) round of review from my side. Biggest missing thing here, I believe, is fuzzy testing. The task it tries to solve should be easy to randomly generate and test the hell out of it.

Reviewed 1 of 3 files at r5, 9 of 11 files at r6, 2 of 2 files at r7. Review status: all files reviewed at latest revision, 23 unresolved discussions.

.travis.yml, line 5 at r7 (raw file):

  - stable
  - beta
  - nightly

gotta except staging.tmp branch

Cargo.toml, line 13 at r7 (raw file):


[dependencies]
gfx-hal = { version = "0.1.0", git = "https://github.com/gfx-rs/gfx.git" }

let's put a specific revision

README.md, line 32 at r7 (raw file):

type SmartBlock<B> = <SmartAllocator<B> as MemoryAllocator<B>>::Block;

fn make_vertex_buffer<B: Backend>(device: &B::Device,

indentation is incorrect

README.md, line 42 at r7 (raw file):

    let reqs = device.get_buffer_requirements(&ubuf);
    // Allocate block of device-local memory that satisfy requirements for buffer.
    let block = allocator.alloc(device, (Type::General, Properties::DEVICE_LOCAL), reqs).map_err(Box::new)?;

I wonder if we'd be fine with accepting a generic parameter being an unbound buffer/image so that get_xxx_requirements can be called automatically. There is not much used of it for the user other than passing to allocator

src/arena.rs, line 17 at r7 (raw file):

/// - `A`: allocator used to allocate bigger blocks of memory
#[derive(Debug)]
pub struct ArenaAllocator<B: Backend, A: MemoryAllocator<B>> {

could be ArenaAllocator<N> for a generic unbound N

src/arena.rs, line 142 at r7 (raw file):

            }
            len if len > index => {
                self.nodes[index].free(block);

could possibly refactor this as match self.nodes.get_mut(index) {..}

src/arena.rs, line 150 at r7 (raw file):


    fn is_used(&self) -> bool {
        self.nodes.is_empty()

why is nodes.is_empty required for is_used?

src/arena.rs, line 162 at r7 (raw file):

        } else {
            if let Some(hot) = self.hot.take() {
                hot.dispose(owner, device).unwrap();

should the error be preserved and propagated here? otherwise, an expect with some message would help

src/arena.rs, line 170 at r7 (raw file):


#[derive(Debug)]
struct ArenaNode<B: Backend, A: MemoryAllocator<B>> {

similarly, only dispose method actually needs to know about A. Everything else could be done by just having ArenaNode<K>, where K is unbound generic parameter for the block. Note: K would be bound by a specific trait for alloc/free implementations.

src/block.rs, line 37 at r7 (raw file):

    {
        use std::ptr::eq;
        eq(self.memory(), other.memory()) && self.range().start <= other.range().start

nit: let's split into 3 lines

src/block.rs, line 52 at r7 (raw file):

/// - `T`: tag type, used by allocators to track allocations
#[derive(Debug)]
pub struct TaggedBlock<B: Backend, T> {

I'm a little bit confused about where this would be needed by the user. It appears that tag is sorta out of place here. It doesn't help anything, and we just end up with a bunch of exposed methods to be able to manage it. Perhaps, we could instead just expose a raw block and then whoever needs to tag it would deal with a tuple in form of (RawBlock<B>, Tag1, Tag2, ...)?

src/chunked.rs, line 15 at r7 (raw file):

    chunks_per_block: usize,
    chunk_size: u64,
    free: VecDeque<(usize, u64)>,

would be nice to have some type names (or simple types) for those, since it's not clear what the numbers are

src/chunked.rs, line 92 at r7 (raw file):

            return Err(MemoryError::NoCompatibleMemoryType);
        }
        if let Some(block) = self.alloc_no_grow() {

nit: let's match here instead of if let/else

src/chunked.rs, line 204 at r7 (raw file):

        debug_assert!(size <= self.max_chunk_size);
        let bits = ::std::mem::size_of::<usize>() * 8;
        assert!(size != 0);

nit: assert_ne!

src/combined.rs, line 15 at r7 (raw file):

    /// For short-lived objects, such as staging buffers.
    ShortLived,

nit: extra space?

src/combined.rs, line 56 at r7 (raw file):

        arena_size: u64,
        chunks_per_block: usize,
        min_chunk_size: u64,

nit: use Range ?

src/combined.rs, line 164 at r7 (raw file):

#[derive(Debug, Clone, Copy)]
pub enum Tag {
    Arena(::arena::Tag),

let's not use the global type paths please?

src/factory.rs, line 129 at r7 (raw file):

    fn range(&self) -> Range<u64> {
        let offset = self.block.range().start;
        offset..offset + self.size

I find it confusing when the range is not surrounded by spaces but the expressions inside are

src/lib.rs, line 5 at r7 (raw file):

//! ### Example
//!
//! ```

Should be marked with rust for better documentation highlights. Not sure if/how it's tested without this

src/smart.rs, line 35 at r7 (raw file):

        arena_size: u64,
        chunks_per_block: usize,
        min_chunk_size: u64,

similarly, can we use native ranges?

src/smart.rs, line 88 at r7 (raw file):

            })
            .filter(|&(_, &mut (ref memory_type, _))| {
                compatible_count += 1;

perhaps we could collect into a small vec in-between the filters to make this a little bit less stateful and more clean?

src/smart.rs, line 153 at r7 (raw file):

/// `free` to find the memory node the block was allocated from.
#[derive(Debug, Clone, Copy)]
pub struct Tag(usize, ::combined::Tag);

global type paths

Comments from Reviewable

Review status: 3 of 12 files reviewed at latest revision, 23 unresolved discussions.

README.md, line 32 at r7 (raw file):