Implement Clownfish Method class

Details

Description

As discussed on lucy-dev, it would be nice to have a Clownfish class for methods. The VTables would then contain a VArray of Methods which can be used to setup host language overrides and later to lookup offsets dynamically.

I chuckled at the "Bootstrap VTables" hacks necessary when the the parcel
prefix is "lucy_" (which will eventually change to "cfish_") so that
VTable_init() could be generalized out of VTable_bootstrap().

Nice adding comments in the generated code. That makes both the transpiler
output and the sprintf format string easier to grok.

FWIW, the variable "x" which is "reserved for future use" in VTable can go
away. It's an artifact of when OFFSET vars had fixed values.

I think it makes sense for Method objects to become immutable, like
VTables (modulo the mutable host object bug for VTables). That would mean
overriding Inc_RefCount() and Dec_RefCount() and leaking them
intentionally.

It's a long-term concern that VTables now contain more elements that aren't
immutable, e.g. VArrays. Our goals for thread support are minimal – we
only support strongly divorced concurrency – but if VTables and their
components are not immutable, we'll have thread safety issues. Perhaps we
need classes like "FinalString", "FinalHash", etc. to support situations
like this.

I notice that the Method takes ownership of the CharBuf "name" in
Method_init() in the third patch. What we do elsewhere is pass in a "const
CharBuf*" and CB_Clone() it, so that the caller doesn't have to worry about
whether they modify it. The same thing applies to VTable_init() in patch
4.

CB_newf(name) might be better written as CB_newf("%s", name), since the
first arg is a format string.

Marvin Humphrey
added a comment - 02/May/12 19:14 Looks good, +1 to commit!
Comments:
The METHOD_PTR patch looks fine.
I chuckled at the "Bootstrap VTables" hacks necessary when the the parcel
prefix is "lucy_" (which will eventually change to "cfish_") so that
VTable_init() could be generalized out of VTable_bootstrap().
Nice adding comments in the generated code. That makes both the transpiler
output and the sprintf format string easier to grok.
FWIW, the variable "x" which is "reserved for future use" in VTable can go
away. It's an artifact of when OFFSET vars had fixed values.
I think it makes sense for Method objects to become immutable, like
VTables (modulo the mutable host object bug for VTables). That would mean
overriding Inc_RefCount() and Dec_RefCount() and leaking them
intentionally.
It's a long-term concern that VTables now contain more elements that aren't
immutable, e.g. VArrays. Our goals for thread support are minimal – we
only support strongly divorced concurrency – but if VTables and their
components are not immutable, we'll have thread safety issues. Perhaps we
need classes like "FinalString", "FinalHash", etc. to support situations
like this.
I notice that the Method takes ownership of the CharBuf "name" in
Method_init() in the third patch. What we do elsewhere is pass in a "const
CharBuf*" and CB_Clone() it, so that the caller doesn't have to worry about
whether they modify it. The same thing applies to VTable_init() in patch
4.
CB_newf(name) might be better written as CB_newf("%s", name), since the
first arg is a format string.

Here is a reworked patchset (0101-0105) addressing some of your comments. Patch 0105 reworks the VTable initialization even more. I think it's best to first look at the generated code in autogen/sources/parcel.c to understand the changes. IMO it makes the whole initialization process more flexible.

Now that VTables are allocated on the heap, I'm not sure if overriding Inc_RefCount and Dec_RefCount is still needed. I think we could and should use a normal destructor now. The same for Methods.

Nick Wellnhofer
added a comment - 08/Jul/12 19:22 Here is a reworked patchset (0101-0105) addressing some of your comments. Patch 0105 reworks the VTable initialization even more. I think it's best to first look at the generated code in autogen/sources/parcel.c to understand the changes. IMO it makes the whole initialization process more flexible.
Now that VTables are allocated on the heap, I'm not sure if overriding Inc_RefCount and Dec_RefCount is still needed. I think we could and should use a normal destructor now. The same for Methods.

Nice improvements! I like how lucy_bootstrap_parcel() has become shorter and
easier to grok now that it has a loop instead of a couple hundred function
calls. And thanks for addressing the handful of concerns from the initial
go-round.

+1 to commit the second patch sequence.

> Now that VTables are allocated on the heap, I'm not sure if overriding
> Inc_RefCount and Dec_RefCount is still needed. I think we could and should
> use a normal destructor now. The same for Methods.

Hmm, that would make these objects mutable – and that poses a problem, as
they are shared globals. Clownfish is designed to support strongly divorced
concurrency in threads; VTables – and now Methods – are shared, but they are
the only objects which can be shared. Having VTables be shared singletons
is a deeply-baked-in assumption for the current design. If we make them
mutable, we will still need to override all refcount manipulation and access
methods, to provide for threadsafe incrementing and decrementing – the
default refcount manipulation methods inherited from Obj are not atomic.

We would also have problems if VTables or Methods were to be freed before any
objects which hold references to them. This can happen under out-of-order
refcount decrementing during Perl's global destruction phase:http://markmail.org/message/52i2n7f2ma5jlajc

Marvin Humphrey
added a comment - 09/Jul/12 23:50 Nice improvements! I like how lucy_bootstrap_parcel() has become shorter and
easier to grok now that it has a loop instead of a couple hundred function
calls. And thanks for addressing the handful of concerns from the initial
go-round.
+1 to commit the second patch sequence.
> Now that VTables are allocated on the heap, I'm not sure if overriding
> Inc_RefCount and Dec_RefCount is still needed. I think we could and should
> use a normal destructor now. The same for Methods.
Hmm, that would make these objects mutable – and that poses a problem, as
they are shared globals. Clownfish is designed to support strongly divorced
concurrency in threads; VTables – and now Methods – are shared, but they are
the only objects which can be shared. Having VTables be shared singletons
is a deeply-baked-in assumption for the current design. If we make them
mutable, we will still need to override all refcount manipulation and access
methods, to provide for threadsafe incrementing and decrementing – the
default refcount manipulation methods inherited from Obj are not atomic.
We would also have problems if VTables or Methods were to be freed before any
objects which hold references to them. This can happen under out-of-order
refcount decrementing during Perl's global destruction phase:
http://markmail.org/message/52i2n7f2ma5jlajc