Skip to content
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

Request: Support fields on foreign classes #994

Open
joshgoebel opened this issue Apr 28, 2021 · 11 comments
Open

Request: Support fields on foreign classes #994

joshgoebel opened this issue Apr 28, 2021 · 11 comments

Comments

@joshgoebel
Copy link

joshgoebel commented Apr 28, 2021

Is your request related to a specific problem you're having?

Yes. I'm work on networking for Wren CLI and the server object needs to hold some state on the C side. It also has some Wren state that is of course stored in fields. Evidently this is currently impossible so you're forced to create two classes, the Wren class and then the C foreign class - and then the Wren class must wrap the foreign class. This is because foreign classes currently do not allow fields. So you end up with something like:

class TCPServer {
    construct new(ip, port) {
        // ...
        _uv = UVServer.new(ip, port, this)
    }
    listen() { _uv.listen_() }
    stop() { _uv.stop_() }
}

foreign class UVServer {
    construct new(ip,port,serverWren) {}
    foreign listen_()
    foreign stop_()
}

This starts to feel very unwieldy. TCPServer now has to keep a reference to UVServer and vice versa (so some things can possibly be passed up the stack). And many functions (anything that requires the lower-level C data) have to be proxied down from the non-foreign class.

Perhaps UVServer will get more complex but so far I feel like I'm asked to create a whole second class and all this proxying complexity soley to attach a some small amount of data to each Wren instance on the C side. Am I missing something? Is there a better pattern here I'm not aware of?

The solution you'd prefer / feature you'd like to see added...

It would be nice if this restriction was removed - if a class could both be foreign AND have fields. A foreign class's RAM allocation might this look like:

[
  WREN_DATA (fields, etc),
  C_DATA (ram allocated to C)
]

Of course this wouldn't have to be contiguous memory, that would be an implementation detail.

Any alternative solutions you considered...

  1. Passing a pointer from C back into Wren as a number. This would of course be problematic for GC but if C ALSO holds onto the object in question with a handle - this would ensure it's never GCed. This pushes some memory management back into Wren as then you'd need to have a destroy callback that passed the number back to C again where the memory could deallocated and then the handle returned via wrenReleaseHandle.

I haven't tried this, it's just what popped into my head.

  1. Perhaps only using the foreign class as a data wrapper and NOTHING more... I have a feeling that might just push the proxying complexity into main class, but on the C side? Example: to close the connection I need the data structures from the C side... so if close was a foreign method on TCPServer the first thing it would have to do is access _uv to get the underlying data so that it can close the connection. I suppose this pattern could be abstracted a bit and perhaps would turn out better. I'm unsure.

  2. Or taken even further:

class TCPServer {
    construct new(ip, port) {
        // create storage on the C side
        _cdata = CData.new(type)
        // do any initial setup of that storage needed on the C side
        c_init_()
    }
    foreign listen() 
    foreign stop()
    foreign c_init_()
    c_data_getter { _cdata } // so _cdata can be accessed from C
}

foreign class CData {
    construct new(type) {}
}

CData's <allocate> function could look at type (a string) and then decide how much memory needs to be allocated. The memory would only be typecast back to whatever useful structure is necessary in the foreign functions on TCPServer.

And then on the C side:

void tcpTCPServerStop(WrenVM* vm) {
    // TCPServe instance is in slot 0
    // using handle we have to capture at system startup
    wrenCall(vm, c_data_getter);
    tcp_server_t* tcpServer = (tcp_server_t*)wrenGetSlotForeign(vm, 0);
    // ...
}

Not terrible. I don't love any of these as much as the idea of just having fields though. :)

Additional context...

None.

@mhermier
Copy link
Contributor

I have this problem in mind since a while, tried various solutions that where not enougth generic for me. The solution without being completely generic is to fusion ObjForeign and ObjInstance structures, and change the API accordingly...

@joshgoebel
Copy link
Author

joshgoebel commented Apr 28, 2021

to fusion ObjForeign and ObjInstance structures, and change the API accordingly

That sounds about right to me. I don't know much about the internals yet so I was more focusing on describing the problem a I saw it :-) I wonder if there is a problem simply allowing create/destroy callbacks on ANY/every class... performance? If that was done then any class that wanted could attach some C data and the foreign distinction would cease to matter at the class level.

@mhermier
Copy link
Contributor

I redid the fusion patch with a unified object as:

typedef struct
{
  Obj obj;
  size_t fieldsSize;
  size_t dataSize;
  Value fields[FLEXIBLE_ARRAY];
//  uint8_t data[FLEXIBLE_ARRAY];
} ObjMemorySegment;

And I already start to see some performances degradations:

api_call - wren                .......... 0.10s 0.0014  98.63% relative to baseline
api_foreign_method - wren      .......... 0.43s 0.0066  93.76% relative to baseline
binary_trees - wren            .......... 0.32s 0.0047  91.81% relative to baseline
binary_trees_gc - wren         .......... 1.29s 0.0183  95.68% relative to baseline
delta_blue - wren              .......... 0.27s 0.0072  99.96% relative to baseline
fib - wren                     .......... 0.40s 0.0072  99.05% relative to baseline
fibers - wren                  .......... 0.05s 0.0008  99.88% relative to baseline
for - wren                     .......... 0.16s 0.0050  99.54% relative to baseline
method_call - wren             .......... 0.21s 0.0050 100.56% relative to baseline
map_numeric - wren             .......... 1.41s 0.0050  98.21% relative to baseline
map_string - wren              .......... 0.14s 0.0024 102.82% relative to baseline
string_equals - wren           .......... 0.27s 0.0058  99.17% relative to baseline

@mhermier
Copy link
Contributor

I made some progress, allowing foreign class to inherit from class with attributes, there is a initialization issue thought.
So the only viable scenarios for now is foreign that inherit from foreign or from class without attributes...

@joshgoebel
Copy link
Author

Well, that's a great start. :-)

there is a initialization issue thought.

What's the issue? Wouldn't a subclass simply be the RAM allocation of each of it's prior classes in the inheritance chain combined? I think foreign class -> foreign class inheritance is a bit of an edge case... and I see issues because technically you don't know the sizes in upfront (they could even vary during runtime!)... I assume you're running into something like that?

@mhermier
Copy link
Contributor

mhermier commented Apr 28, 2021

The issue is that you can't call an initialize method from the super class, since it would meant re-entrancy. So basically if it is mandatory to initialize an object you are doomed...

In the case of foreign sub-classing a foreign class, you are in control and can call a super initialize function in C from C.

@joshgoebel
Copy link
Author

The issue is that you can't call an initialize method from the super class, since it would meant re-entrancy.

Would you mind giving an example of what wouldn't work?

@mhermier
Copy link
Contributor

class Foo {
  construct new() {
    _secret = SecretFactory.construct()
  }
  ...
}

foreign class Bar is Foo {
  foreign construct new() // here in C you can't call Foo.new(), therefore since Foo rely on _secret to be
                          // initialized by the constructor to function correctly, you end up in a limbo state
}

@joshgoebel
Copy link
Author

I see now. Yeah, if we had everything except that single edge case I think that'd be a huge improvement. :-)

@mhermier
Copy link
Contributor

What should be the behavior around inheritance of allocate & finalize ?

  • NULL means use parent.
  • NULL is error / no allocate (can be useful if an intermediate class is not constructible?)
  • something else ?

@joshgoebel
Copy link
Author

Ruby might be better to answer that one... In my experience not doing the allocation is very bad mojo, but I don't know enough to know if that could or should be different for inheritance. I would say the simpler thing would be to simply always require it. In my understanding this is how the actual storage for the underlying class is created - the VM doesn't do that on it's own, though I suppose those are the bits you're fiddling with now.

I mean I suppose you could do the callback and then not allocate and expect the VM to do it for you, but then why are you using a foreign class at all? If you only needed a callback you could do that with a foreign method called from inside your new method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants