On 04/08/2014 22:37, Marvin Humphrey wrote:
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().
OK with me. It will only be surprising for anyone who really needs to clone a
String and doesn't know that String is special in that regard.
- 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);
No, it's safe. String#Inc_RefCount returns a new object for "wrapper" strings
that don't own the char buffer (string->origin == NULL). StackStrings are
always wrappers. (This also means that Inc_RefCount has to stay a method even
with the immortal flag.)
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.)
OK, let's use a raw array.
- 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.
Good point.
It depends on what guarantees LFReg#Clone should make exactly. I think it's OK
if it returns a clone of the registry in some arbitrary state during the
cloning process. For example, if a single object is inserted during the
cloning process, it would be OK to return a clone without that object. The
registry could change immediately after the final size check anyway.
But my implementation could return a version of the registry in a completely
different state. If, during cloning, object A is inserted first and then
object B, a version with only object B could be returned. This is clearly a
bug, so let's skip that commit.
Iteration, in contrast, simply cannot defend against concurrent modifications
without locking. And therefore this implementation suffices. Very nice,
actually!
Iteration suffers from the same problem mentioned above. This could, for
example, lead to finding a class but not its parent.
This is all more complicated than I initially thought, so I'll leave LFReg as
is.
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.
+1 for Get_Key() and Get_Value(). I'm also for separate Iterator objects in
general. But let's ignore that for now.
Nick
