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

Pass callback from host to runtime for getting properties from the host #77458

Closed
Tracked by #77456
elinor-fung opened this issue Oct 25, 2022 · 11 comments · Fixed by #78798
Closed
Tracked by #77456

Pass callback from host to runtime for getting properties from the host #77458

elinor-fung opened this issue Oct 25, 2022 · 11 comments · Fixed by #78798
Milestone

Comments

@elinor-fung
Copy link
Member

The host passes information to the runtime via a string to string mapping of properties. Adding a property represents a non-trivial cost, but is currently the only way we have to pass more information from the host to the runtime.

We should add a different mechanism for this. We can pass over a callback (similar to what we currently do BUNDLE_PROBE and PINVOKE_OVERRIDE) for getting properties. Something like:

size_t STDMETHODCALLTYPE get_runtime_property(
    const char* key,
    void* value_buffer,
    size_t value_buffer_size)

The value_buffer actual type would depend on the key, such that we could have structured information.

This would enable us to provide the information necessary to support:

Runtimes need to continue to respect the existing properties, but once the callback exists, it could also be used for existing properties as appropriate. This would be a step towards:

@elinor-fung elinor-fung added this to the 8.0.0 milestone Oct 25, 2022
@ghost
Copy link

ghost commented Oct 25, 2022

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

The host passes information to the runtime via a string to string mapping of properties. Adding a property represents a non-trivial cost, but is currently the only way we have to pass more information from the host to the runtime.

We should add a different mechanism for this. We can pass over a callback (similar to what we currently do BUNDLE_PROBE and PINVOKE_OVERRIDE) for getting properties. Something like:

size_t STDMETHODCALLTYPE get_runtime_property(
    const char* key,
    void* value_buffer,
    size_t value_buffer_size)

The value_buffer actual type would depend on the key, such that we could have structured information.

This would enable us to provide the information necessary to support:

Runtimes need to continue to respect the existing properties, but once the callback exists, it could also be used for existing properties as appropriate. This would be a step towards:

Author: elinor-fung
Assignees: -
Labels:

area-Host

Milestone: 8.0.0

@elinor-fung elinor-fung mentioned this issue Oct 25, 2022
10 tasks
@vitek-karas
Copy link
Member

Definitely like this approach - as mentioned this would allow the runtime to only ask for a given property when it needs it. Some properties are never needed, or are only needed much later on, thus improves startup perf.

In terms of the API - I think the interesting question is memory ownership. As proposed this would mean that the caller (runtime) allocates the memory and the callee (host) copies data into it. The other approach would be for the host to return a pointer into the "host's" memory with the data in it.

I think it would be interesting to describe pros/cons of the two approaches, what comes to mind:

  • runtime allocated memory -> very likely a second copy will need to be made in runtime's memory to convert the data into whatever shape runtime needs it in (specifically strings being converted from UTF8 -> UTF16).
  • host allocated memory -> no good way to free that memory - host would have to hold onto it forever. That's what it does today, so in that sense it would be faster, but maybe not ideal going forward.

@jkotas
Copy link
Member

jkotas commented Oct 26, 2022

How is this going to interact with AppContext.Get/SetData? All host provided properties show up in AppContext key/value pairs today.

@elinor-fung
Copy link
Member Author

In terms of the API - I think the interesting question is memory ownership.

As you noted, I had runtime-allocated memory in mind. I wanted to avoid any issues with new/delete differences between host/runtime and didn't want to force keeping every requested property around. With the value being a void*, I figured that also gave us the flexibility to do something like have a struct field point at memory managed by the host.

How is this going to interact with AppContext.Get/SetData? All host provided properties show up in AppContext key/value pairs today.

That would need to continue being the case. For the well-known host properties (which I expect are not directly accessed by most applications), I'd want them to be queried/constructed on-demand. It would mean having knowledge of the relevant property names, but I think it would be worth it if we can avoid something like having the giant TPA string around unnecessarily.

@vitek-karas
Copy link
Member

Makes sense.

For the AppContext.Get/SetData ideally we would change that to call into the host when it's asked about a property - and probably cache the result (not sure about that). The interesting question is what do we do for properties which don't have nice values. Currently we tunnel the function pointers via strings, so there's something to show in GetData, but if start using true pointers as property values, what would be the return value of GetData?
The other problem is that if the runtime doesn't recognize the property, it would have no idea what is the data type. Maybe we should add something to the new API to at least differentiate string values - so that runtime can populate those to GetData without issues.

@jkotas
Copy link
Member

jkotas commented Oct 28, 2022

I think it would be worth it if we can avoid something like having the giant TPA string around unnecessarily.

The TPA string comes with corner-case functional issues today. If the app path contains :, the app won't start:

jkotas:~/repro$ bin/Debug/net6.0/repro
Hello, World!
jkotas:~/repro$ mv bin/ :/
jkotas:~/repro$ :/Debug/net6.0/repro
Failed to create CoreCLR, HRESULT: 0x80070057

We may want to rethink the interface for the TPA list as part of this to fix all known functional and performance issues.

@elinor-fung
Copy link
Member Author

For the AppContext.Get/SetData ideally we would change that to call into the host when it's asked about a property - and probably cache the result (not sure about that).

Yeah, I think we'd need to investigate / determine if we care about any potential perf implications when we get to that though (always calling down / caching / creating an initial list of names and only calling down for those, etc)

The other problem is that if the runtime doesn't recognize the property, it would have no idea what is the data type. Maybe we should add something to the new API to at least differentiate string values - so that runtime can populate those to GetData without issues.

I do like the idea of having some sort of out int/enum value to indicate the type. Something like:

size_t STDMETHODCALLTYPE get_runtime_property(
    const char* key,
    uint8_t* value_type,
    void* value_buffer,
    size_t value_buffer_size)

We don't have (or plan to have?) a way for users to specify non-string properties right now (right?), so this would more be for host-provided properties, or for anyone not going through hostpolicy?

We may want to rethink the interface for the TPA list as part of this to fix all known functional and performance issues.

I definitely think that this should enable rethinking properties like the TPA list (also the other lookup paths, but I think the TPA is the worst offender). Those are ones I'd like to pass over as something structured such that the host doesn't need to build up a long string (with a separator tha potentially causing issues) that the runtime then needs to parse back into the separate paths. For compatability, we could create their current form only if their property names get requested via GetData.

@jkotas
Copy link
Member

jkotas commented Oct 28, 2022

I do like the idea of having some sort of out int/enum value to indicate the type.

Once you get to passing structured non-textual data around, it may be better to think about the host/runtime interface as a classic strong-typed COM-like interface.

@elinor-fung
Copy link
Member Author

As in the void* value returned is actually some basic type that you can query for what it actually is (like IUnknown/QI)? Or am I missing what you are saying?

@jkotas
Copy link
Member

jkotas commented Oct 28, 2022

Let me ask differently. PINVOKE_OVERRIDE is an example of structured data. Once this proposed callback is in place and we need to add another callback like PINVOKE_OVERRIDE, what is it going to look like if we were to describe the structured data using enum?

@elinor-fung
Copy link
Member Author

I'd see passing over a contract / table of function pointers rather than a single function pointer.

struct host_contract
{
    size_t version;
    bool(STDMETHODCALLTYPE* bundle_probe)(...);
    void* (STDMETHODCALLTYPE* pinvoke_override)(...);
    size_t(STDMETHODCALLTYPE* get_runtime_property)(...);
    int32_t(STDMETHODCALLTYPE* other_operation)(...);
};

where we could provide some of the well-known properties via a separate function with the actual type of the data if desired.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 6, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Dec 6, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jan 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants