support for custom allocator in ByteBuffer

[ghost]

Adds possibility to allocate memory for byte buffers with something other than default malloc.
@thomasfanell or @emilwestergren review

[emilwestergren]

Should this really be static? What if you want several different allocators?

[ghost]

How would the api look like if this was per-instance ? ByteBuffer new(128, |size| myAlloc(size) ) ?
Then how do we know when to return a recycled instance, as they could be allocated with different allocators ?
We can think about it later of course, but for now I'd like to allocate buffers by delegating allocation to some external library that has a pool of preallocated memory aligned on 16 byte boundary.
This is more flexible than hardcoded malloc anyway :)

[ghost]

Wait, I want a custom free too.

[emilwestergren]

I guess all allocations would have to go through an allocator instance and not by simply using constructor.

[thomasfanell]

Nice one. Are you thinking _mm_malloc?

[ghost]

Alright, so maybe

AbstractAllocator: abstract class {
    allocate: abstract func (size: SIzeT) -> Void*
    deallocate: abstract func (pointer: Void*)
}

...

MallocAllocator: class extends AbstractAllocator {
    allocate: override func (size: SizeT) -> Void* { malloc(size) }
    deallocate: override func (pointer: Void*) { memfree(pointer) }
}

....
ByteBuffer: class {
    ...
    _allocator := MallocAllocator new()

   init: func ~withAllocator (size: SizeT, allocator: AbstractAllocator)
}

should work ?

[ghost]

@thomasfanell : I need that for some external library, but I have no idea what they are using under the hood.

[thomasfanell]

Would it be worth adding something like this then?

AlignedMemoryAllocator: class extends AbstractAllocator {
    alignment: SizeT
    init: func (=alignment)
    allocate: override func (size: SizeT) -> Void* { _mm_malloc(size, this alignment) }
    deallocate: override func (pointer: Void*) { _mm_free(pointer) }
}

[ghost]

I guess so, there is also aligned_alloc in C11 standard, so maybe we can use that after our default gcc version is updated.

[emilwestergren]

You don't have to implement the Allocator instance here though, I was just thinking there will probably be times where you have different allocation requirements in different parts of the code, and then a static allocator will not be enough.

[ghost]

Yeah, I agree. I'm almost done with this anyway.

[ghost]

For some reason allocator: AbstractAllocator = null does not compile here, it is somehow affected by the previous false default argument:

base/ByteBuffer.ooc:24:47 error Couldn't unwrap decl false: AbstractAllocator , trail =
_, Module ByteBuffer
_, ClassDecl ByteBuffer
_, ClassDecl ByteBufferClass
_, static ByteBuffer new(=_pointer, =_size, ownsMemory: = (false: AbstractAllocator, allocator: AbstractAllocator = null)) -> ByteBuffer
_, ownsMemory: = (false: AbstractAllocator, allocator: AbstractAllocator = null)
_, (false: AbstractAllocator, allocator: AbstractAllocator = null)

[thomasfanell]

Yes, rock does not handle this correctly: ooc-lang/rock#990

[ghost]

Updated. @thomasfanell can you review again ?

[emilwestergren]

Should the allocator really be owned by the buffer?

[ghost]

I guess that's easiest for now. Allocator could also have a Owner flag.

[thomasfanell]

I think it's quite natural for the buffer to take ownership of the allocator. @emilwestergren can you think of a case were this would be undesirable?

[emilwestergren]

It's not uncommon to have an allocator as a singleton for example. In most cases you will maybe have a few different allocators, and to allocate and free these for every ByteBuffer seems like a waste. We could for example have a few static standard allocators in the ByteBuffer or Allocator class.

buffer := ByteBuffer new(size, Allocator malloc)

[ghost]

Yes and a possibility to override the default allocator, as my point here was to allow custom memory allocation for byte buffers in raster image classes. This is currently not possible, but for the sake of code quality I happily sacrifice my own personal short-sighted goals.

[thomasfanell]

Good point

[ghost]

Added Owner flag. Review again ? :)

[emilwestergren]

Void* is called Pointer in ooc

[emilwestergren]

Another way is to simply have a concrete Allocator class where you can supply allocate/deallocate closures. That way you don't have to inherit the class for every single allocator, only if you need to add more behaviour. Also, then we can add static default allocators in this class.

Allocator: class 
mallocAllocator := static This new( func { malloc ... }, func { free... })

[ghost]

Good idea. I agree that allocating allocators for every object seems unnecessary.

I shall add default allocators now to prevent this.

[alexnask]

You can also use template classes to create composable allocators (I'm thinking about Andrei Alexandrescu's design of the D std.allocator), although they are quite broken right now.

By the way, another nice way to expose allocators to any ooc code would be to add a global current_allocator variable and use it to allocate and free (in Object alloc and Object free) although that means that you then have to keep track of what variables were allocated with what allocator etc.

With such a design, I expect you would use a single well tuned allocator for most allocations and switch allocators in specific places (for example, current_allocator = SomePoolAllocator; /* allocate thousands of small objects here, use them, free them */; current_allocator = SystemWideAllocator).

EDIT: Also, you could then do stuff like that:

with_allocator(PoolAllocator, ||
    // Do stuff
)

// Better (avoids closure)
switch_allocator(PoolAllocator)

// Do stuff
reset_allocator()

[ghost]

Thanks @Shamanas
use template classes
⬆️ that was the first thing I though of, but I gave up after fighting some compiler errors :/ This here is not perfect, but suits my needs for now. You have some nice ideas here, I think we may revisit this someday.

[alexnask]

@sebastianbaginski
Yeah template classes definitely do need a whole lot of patching, althoug hthe only error I can currently think of right now (which is pretty big itself) is that modules that define them need to import all modules of the types they are instanciated with.

[emilwestergren]

Looks good to me, just had some ideas/opinions but this is fine anyhow.

[ghost]

@thomasfanell @emilwestergren can you take a look at the latest changes ?

[emilwestergren]

Looks good to me

[thomasfanell]

Looks good

[marcusnaslund]

Since it would be possible to use Allocator for other purposes besides ByteBuffer, I'd suggest not freeing it here but rather adding its own explicit register call to GlobalCleanup.
...the only bothersome thing is that the order is important (it needs to happen after ByteBuffer free~all()).
GlobalCleanup register(|| Allocator free~all(), true) puts the call at the bottom of the (current) stack. If all else fails just put it after line 67 of this file.

[ghost]

I think I've tried that and the order was messed up. Maybe we could use a priority integer value instead of bool to control the cleanup order.
But ok, for now I can put it after the line 67.

[marcusnaslund]

Thanks.
#1848