Lucy
  1. Lucy
  2. LUCY-233

Implement Clownfish Method class

    Details

    • Type: Task Task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Clownfish
    • Labels:
      None

      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.

      1. take1_take2.diff
        12 kB
        Marvin Humphrey
      2. 0105-Autogenerate-an-array-with-initialization-data-for-a.patch
        28 kB
        Nick Wellnhofer
      3. 0104-Rework-VTable-method-initialization.patch
        20 kB
        Nick Wellnhofer
      4. 0103-Initial-implementation-of-Method-class.patch
        14 kB
        Nick Wellnhofer
      5. 0102-Rework-VTable-bootstrapping.patch
        10 kB
        Nick Wellnhofer
      6. 0101-Rename-METHOD-macro-to-METHOD_PTR.patch
        31 kB
        Nick Wellnhofer
      7. 0004-Rework-VTable-method-initialization.patch
        19 kB
        Nick Wellnhofer
      8. 0003-Initial-implementation-of-Method-class.patch
        13 kB
        Nick Wellnhofer
      9. 0002-Rework-VTable-bootstrapping.patch
        10 kB
        Nick Wellnhofer
      10. 0001-Rename-METHOD-macro-to-METHOD_PTR.patch
        31 kB
        Nick Wellnhofer

        Activity

        Nick Wellnhofer created issue -
        Hide
        Nick Wellnhofer added a comment -

        Patches 01-04 contain an initial implementation of the Method class and a rework of the autogenerated VTable initialization code.

        Show
        Nick Wellnhofer added a comment - Patches 01-04 contain an initial implementation of the Method class and a rework of the autogenerated VTable initialization code.
        Nick Wellnhofer made changes -
        Field Original Value New Value
        Attachment 0001-Rename-METHOD-macro-to-METHOD_PTR.patch [ 12525192 ]
        Attachment 0002-Rework-VTable-bootstrapping.patch [ 12525193 ]
        Attachment 0003-Initial-implementation-of-Method-class.patch [ 12525194 ]
        Attachment 0004-Rework-VTable-method-initialization.patch [ 12525195 ]
        Hide
        Marvin Humphrey added a comment -

        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.
        Show
        Marvin Humphrey added a comment - 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.
        Hide
        Nick Wellnhofer added a comment -

        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.

        Show
        Nick Wellnhofer added a comment - 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.
        Hide
        Marvin Humphrey added a comment -

        The attached file "take1_take2.diff" shows the difference between
        the first four patches of patchset 1 and patchset 2.

        Show
        Marvin Humphrey added a comment - The attached file "take1_take2.diff" shows the difference between the first four patches of patchset 1 and patchset 2.
        Marvin Humphrey made changes -
        Attachment take1_take2.diff [ 12535757 ]
        Hide
        Marvin Humphrey added a comment -

        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

        Show
        Marvin Humphrey added a comment - 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
        Hide
        Nick Wellnhofer added a comment -

        OK, now I understand why we need the special refcount handling.

        Show
        Nick Wellnhofer added a comment - OK, now I understand why we need the special refcount handling.
        Hide
        Nick Wellnhofer added a comment -

        Committed the second patch series and an additional commit that disables refcounting of Methods.

        Show
        Nick Wellnhofer added a comment - Committed the second patch series and an additional commit that disables refcounting of Methods.
        Nick Wellnhofer made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Assignee Nick Wellnhofer [ nwellnhof ]
        Resolution Fixed [ 1 ]

          People

          • Assignee:
            Nick Wellnhofer
            Reporter:
            Nick Wellnhofer
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development