Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support for custom allocator in ByteBuffer #1845

Merged
merged 5 commits into from Aug 1, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions base.use
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ Name: Base
Description: Base library that complements the built-in SDK.
SourcePath: source/base
Imports: Order
Imports: Allocator
Imports: ByteBuffer
Imports: DateTime
Imports: TimeSpan
Expand Down
18 changes: 18 additions & 0 deletions source/base/Allocator.ooc
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/* This file is part of magic-sdk, an sdk for the open source programming language magic.
*
* Copyright (C) 2016 magic-lang
*
* This software may be modified and distributed under the terms
* of the MIT license. See the LICENSE file for details.
*/

AbstractAllocator: abstract class {
Copy link
Contributor

@emilwestergren emilwestergren Jul 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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... })

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I shall add default allocators now to prevent this.

Copy link

@alexnask alexnask Jul 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

allocate: abstract func (size: SizeT) -> Void*
deallocate: abstract func (pointer: Void*)
}

MallocAllocator: class extends AbstractAllocator {
init: func
allocate: override func (size: SizeT) -> Void* { malloc(size) }
deallocate: override func (pointer: Void*) { memfree(pointer) }
}
17 changes: 11 additions & 6 deletions source/base/ByteBuffer.ooc
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/

use collections
import Allocator
import ReferenceCounter
import Debug

Expand All @@ -15,21 +16,24 @@ ByteBuffer: class {
_size: Int
_referenceCount: ReferenceCounter
_ownsMemory: Bool
_allocator: AbstractAllocator = null
pointer ::= this _pointer
size ::= this _size
referenceCount ::= this _referenceCount

init: func (=_pointer, =_size, ownsMemory := false) {
init: func (=_pointer, =_size, ownsMemory := false, allocator := null as AbstractAllocator) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this _referenceCount = ReferenceCounter new(this)
this _ownsMemory = ownsMemory
this _allocator = allocator ?? MallocAllocator new() as AbstractAllocator
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cast is needed to prevent warning pointer type mismatch in conditional expression :/

}
free: override func {
if (this _referenceCount != null)
this _referenceCount free()
this _referenceCount = null
if (this _ownsMemory)
memfree(this _pointer)
this _allocator deallocate(this _pointer)
this _pointer = null
this _allocator free()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the allocator really be owned by the buffer?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@emilwestergren emilwestergren Jul 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point

super()
}
zero: func ~whole { this memset(0) }
Expand All @@ -50,7 +54,7 @@ ByteBuffer: class {
copyTo: func (other: This, start: Int, destination: Int, length: Int) {
memcpy(other pointer + destination, this pointer + start, length)
}
new: static func ~size (size: Int) -> This { _RecyclableByteBuffer new(size) }
new: static func ~size (size: Int, allocator: AbstractAllocator = null) -> This { _RecyclableByteBuffer new(size, allocator) }
new: static func ~recover (pointer: Byte*, size: Int, recover: Func (This) -> Bool) -> This {
_RecoverableByteBuffer new(pointer, size, recover)
}
Expand Down Expand Up @@ -86,7 +90,7 @@ _RecoverableByteBuffer: class extends ByteBuffer {
}

_RecyclableByteBuffer: class extends ByteBuffer {
init: func (pointer: Byte*, size: Int) { super(pointer, size, true) }
init: func (pointer: Byte*, size: Int, allocator: AbstractAllocator = null) { super(pointer, size, true, allocator) }
_forceFree: func {
this _size = 0
this free()
Expand All @@ -111,7 +115,7 @@ _RecyclableByteBuffer: class extends ByteBuffer {
_smallRecycleBin := static VectorList<This> new()
_mediumRecycleBin := static VectorList<This> new()
_largeRecycleBin := static VectorList<This> new()
new: static func ~fromSize (size: Int) -> This {
new: static func ~fromSize (size: Int, allocator: AbstractAllocator = null) -> This {
buffer: This = null
bin := This _getBin(size)
This _lock lock()
Expand All @@ -123,7 +127,8 @@ _RecyclableByteBuffer: class extends ByteBuffer {
}
This _lock unlock()
version(debugByteBuffer) { if (buffer == null) Debug print("No RecyclableByteBuffer available in the bin; allocating a new one") }
buffer == null ? This new(malloc(size), size) : buffer
allocator = allocator ?? MallocAllocator new() as AbstractAllocator
buffer == null ? This new(allocator allocate(size) as Byte*, size, allocator) : buffer
}
_getBin: static func (size: Int) -> VectorList<This> {
size < 10000 ? This _smallRecycleBin : (size < 100000 ? This _mediumRecycleBin : This _largeRecycleBin)
Expand Down
20 changes: 20 additions & 0 deletions test/base/ByteBufferTest.ooc
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,18 @@
use base
use unit

CustomAllocator: class extends AbstractAllocator {
_allocCount := 0
init: func
allocate: override func (size: SizeT) -> Void* {
this _allocCount += 1
malloc(size)
}
deallocate: override func (pointer: Void*) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Void* is called Pointer in ooc

memfree(pointer)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe have a freeCounter as well and make sure that we indeed call deallocate.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

}
}

ByteBufferTest: class extends Fixture {
init: func {
super("ByteBuffer")
Expand Down Expand Up @@ -78,6 +90,14 @@ ByteBufferTest: class extends Fixture {
expect(buffer pointer[63] as Int, is equal to(63))
buffer referenceCount decrease()
})
this add("custom alloc", This _testCustomAlloc)
}
_testCustomAlloc: static func {
allocator := CustomAllocator new()
expect(allocator _allocCount, is equal to(0))
buffer := ByteBuffer new(128, allocator)
expect(allocator _allocCount, is equal to(1))
buffer free()
}
}

Expand Down