On Mon, Aug 4, 2014 at 2:42 AM, Nick Wellnhofer <[email protected]> wrote:
> - Make String#Clone create a new String object
>
> This removes an optimization that immutable strings made possible. But
> especially with regard to threads, I think it's important that String#Clone
> really creates a new object and doesn't just increment the refcount.
This is somewhat unfortunate because under hosts with tracing GC the
optimization remains valid. And of course the optimization remains valid
within single threaded execution. I liked what you had before!
Could we instead decide that Clone() is not thread safe? And then in the
context of duplicating `klass->name` and the like we'd use some other
constructor instead of Clone().
> - Special case String hash keys
>
> To make up for the commit above.
The principle of special-casing String keys in Hash for the sake of
optimization is sound because it's the truly common case. But if we don't
make the commit above this commit may not be necessary.
> - Implement Method#Clone
> - Make sure to clone a Class's methods
>
> Not strictly needed right now but a good precaution.
This change to Method_new() isn't safe if someone passes in a StackString,
right?
- self->name = Str_Clone(name);
+ self->name = (String*)INCREF(name);
Aside from that, the commit adding Method_Clone() seems fine in principle.
With regards to the other commit about cloning a Class's methods, it seems
like an appropriate fix for the current implementation. (Ultimately, I
would prefer to use a raw Method** array rather than a VArray here in order to
guarantee immutability.)
> - Implement LFReg#Clone
> - Implement iterator for LockFreeRegistry
Since LockFreeRegistry only supports addition of elements and not replacement
or removal, it is possible to implement Clone() with a lock-free algorithm
that it is truly threadsafe.
* Iterate through all elements, counting the number of iterations.
* When finished, iterate again to confirm that no elements were added by
other threads while the cloning was underway, by ensuring that the total
number of elements hasn't changed.
* If the count *has* changed, iterate again to find and insert any missing
elements.
The second count-only iteration can be avoided if we modify LockFreeRegistry
to track its size.
If we are going to implement functionality to duplicate a LockFreeRegistry, I
think that the implementation should be threadsafe. This is the only
threadsafe class we support which has mutable data aside from refcounts, so
the extra care is justified.
Iteration, in contrast, simply cannot defend against concurrent modifications
without locking. And therefore this implementation suffices. Very nice,
actually!
One thing to think about is that if we add interfaces to Clownfish, we'll have
to normalize the various signatures of Next(), perhaps necessitating that we
access key and value via Get_Key() and Get_Value(). But that applies to
several other iterators as well, so shouldn't block this commit.
Marvin Humphrey
