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

feat: access input/output buffers directly #11

Merged
merged 9 commits into from Sep 12, 2022
Merged

feat: access input/output buffers directly #11

merged 9 commits into from Sep 12, 2022

Conversation

zshipko
Copy link
Contributor

@zshipko zshipko commented Sep 10, 2022

This PR updates extism_output_get to return an actual pointer to the output value (const uint8_t* extism_output_get(PluginIndex plugin) instead of void extism_output_get(PluginIndex plugin, uint8_t *buffer, uint64_t length)), this pointer will only be valid until the next call, but it makes it possible to access the output data without copying.

The only issue I can see is this complicates the lifetime of the output value. In most cases a copy will still be made or the output buffer will only be used until the next call, otherwise it's possible to lose access to an existing output once the same plugin is called again. But this is easily fixed by making a copy in the host program. Overall I think this change will provide a lot of benefit, do you think it's worth it?

EDIT: Now the input buffer is also not copied and the same issue applies - the input buffer must not change during call - I think this is a reasonable constraint though.

@zshipko zshipko changed the title feat: Aceess output buffer directly feat: Aceess input/output buffers directly Sep 10, 2022
@nilslice
Copy link
Member

This is awesome. Nice work!

I've manually tested extism/extism-fsnotify using the new dylib, as well as with re-built plugins in rust, and the assemblyscript PDK example.ts and all works great.

I have not tested logging from these yet, nor trying to use the Pointer type in examples. But, no regressions found! So, I'm good to merge this if you are.

nilslice
nilslice previously approved these changes Sep 11, 2022
@nilslice nilslice changed the title feat: Aceess input/output buffers directly feat: access input/output buffers directly Sep 11, 2022
@nilslice
Copy link
Member

The only issue I can see is this complicates the lifetime of the output value. In most cases a copy will still be made or the output buffer will only be used until the next call, otherwise it's possible to lose access to an existing output once the same plugin is called again. But this is easily fixed by making a copy in the host program. Overall I think this change will provide a lot of benefit, do you think it's worth it?

EDIT: Now the input buffer is also not copied and the same issue applies - the input buffer must not change during call - I think this is a reasonable constraint though.

If I'm understanding the complication, then IMO, this isn't really any different than what you'd need to expect when not using plug-ins and passing a pointer around. I don't think it will be common for a host program to get the output, not use it immediately, then call the same plugin, and expect the previous output to be usable. Same goes for input data.

To clarify:

the input buffer must not change during call

You are saying that the Host cannot change the input buffer? It should be treated as if the ownership is transferred to the call fn. Because once the plugin loads it, it's already copied into WASM, ya? & the plugin can't modify the Host's input buffer anyways.

@zshipko
Copy link
Contributor Author

zshipko commented Sep 11, 2022

You are saying that the Host cannot change the input buffer?

Yeah, if the host were to free the data or something before the plug-in copies the memory it could be an issue, but like you said it’s the same rules for passing a pointer around in any language so it shouldn’t be an issue.

@nilslice
Copy link
Member

free the data or something before the plug-in copies the memory

Ah I see, right.. yea tbh, it's probably pretty hard to do this in most of the Host SDK languages anyways!

@nilslice nilslice merged commit 8bc608d into main Sep 12, 2022
@nilslice nilslice deleted the direct-output branch September 12, 2022 15:34
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

Successfully merging this pull request may close these issues.

None yet

2 participants