-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Draft of smart pointers library for Nim #10485
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
Conversation
| type | ||
| UniquePtr*[T] = object | ||
| ## non copyable pointer to object T, exclusive ownership of the object is assumed | ||
| val: ptr tuple[value: T, allocator: Allocator] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the allocator here but I don't have a better idea either. Maybe a static[proc] type paramter instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this idea. IMO, it is bad if 2 smart pointers will be treated as incompatible types, just because allocator was different.
I see the following options:
- No support for different allocators ;(
- There is only one allocator returned by
getAllocatorand it user responsibility to set it to thread local allocator, global allocator or custom allocator at start up time. - Do what I just did. While overhead seems noticeable, it is possibly less than one might think.
There is 16 byte alignment requirement for all heap allocations on 64 bit platforms, hence you are likely to overallocate anyway hence there is a space for extra pointer. - Do what I just did, but do have mode were
Allocatortype is actuallyvoidand have zero byte size and alloc, dealloc are silently mapped to system'sallocanddealloc. Hence, those users who don't need customisation do not pay for it. Possible ABI compatibility problems due different defined flags used by different libraries. Problems are potentially solvable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some ideas of solving this problem... I'm writing a blog post.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I will wait for it then. Or if it is possible to describe it in one sentence please do so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a pool for the allocations, make the pool's address aligned so that you can bitmask the pointers to access the pool's header which contains the allocator.
| proc `=destroy`*[T](p: var SharedPtr[T]) = | ||
| mixin `=destroy` | ||
| if p.val != nil: | ||
| let c = atomicDec(p.val[].atomicCounter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can perform this atomicDec in an else when you change the check to atomicRead(p.val[].atomicCounter) == 1. And == 0 would be better still, for this you need to store the value of RC-1.
| dest.val = src.val | ||
|
|
||
| proc `=`*[T](dest: var SharedPtr[T], src: SharedPtr[T]) = | ||
| if dest.val != src.val: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The classical algorithm does the incRef first and then the decRef and doesn't require the dest.val != src.val check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will come back on this one. I haven't seen this algo before hence I can't comment now.
| if AllocatorFlag.ZerosMem notin a.flags: | ||
| reset(result.val[]) | ||
| result.val.value = val | ||
| result.val.atomicCounter = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you store RC-1 here this becomes a 0.
lib/pure/collections/smart_ptr.nim
Outdated
| proc get*[T](p: SharedPtr[T]): var T {.inline.} = | ||
| when compileOption("boundChecks"): | ||
| if p.val == nil: | ||
| raise newException(ValueError, "deferencing nil shared pointer") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert here.
The default should be thread-safe, IMO. We need to have a faster threadsafe allocator. But I dislike that every unique and shared pointer has a field to the allocator, we need to do something about that. |
|
Oh and also: If you use the optimized refcounting variant that I outlined there is little need for the |
|
Sorry to be a buzzkill, but this gives me flashbacks of C++ too much. Do we have to adopt this into the stdlib? |
|
If you are using destructor based objects on heap these smart pointers are very helpful. I don't more clever idea than smart pointers. If anyone has please speak up. If you are using |
|
@cooldome The point is that we should/could map |
|
@Araq: Your |
|
@cooldome |
|
Stalled PR, we are all busy with ARC, please reopen if you need it. |
Consume was used in the original pr nim-lang/Nim#10485, haven't questioned before nim-lang/Nim#19212
This is early draft for user interface discussion. I will add tests and more doc once the interface is settled down.
Main design decisions:
.,.=,.()=templates. I have found user experience with converters a lot smoother. Error messages with dot related operators are terrible. Luckily, converters no longer need to copy thanks forvar T,lent Treturn type support.sink Tandlent Tmake_sharedand similar. Nim doesn't have constructors, hence you can't create object in a uniform way. I don't think it is a problem thanks tosink Targuments, it is enough to have single way to create a smart pointernewXXXPtr[T](arg: sink T): Ptr[T]ConstPtr[T]is a distinctSharedPtr[T]is on purpose. User can make copy of smart pointer and pass it futher without a right to mutate the underlying object.Open questions:
getSharedAllocator()and when to callgetLocalAllocator()?