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: add Host Functions support #13

Merged
merged 22 commits into from
Jan 29, 2024
Merged

feat: add Host Functions support #13

merged 22 commits into from
Jan 29, 2024

Conversation

mhmd-azeez
Copy link
Contributor

@mhmd-azeez mhmd-azeez commented Jan 23, 2024

Fixes #11

  • Implement FFI calls
  • Implement CurrentPlugin
  • Implement HostFunction
  • Fail fast when validating callback params
  • Add PHPDoc comments
  • Add README section
  • Add tests
  • Review for breaking changes
  • User Data. Wasn't able to find a way to pin PHP objects and get their address. Users can use captured variables in the callback closures instead.

@mhmd-azeez mhmd-azeez self-assigned this Jan 23, 2024
@mhmd-azeez mhmd-azeez marked this pull request as ready for review January 24, 2024 17:34
@mhmd-azeez mhmd-azeez requested review from zshipko, bhelx and nilslice and removed request for zshipko January 24, 2024 17:35
@nilslice
Copy link
Member

cc @AtlantisPleb

@mhmd-azeez
Copy link
Contributor Author

I will need to do some tests regarding memory leaks, since FFI callbacks seem to leak by default:
https://www.php.net/manual/en/ffi.examples-callback.php

Although this works, this functionality is not supported on all libffi platforms, is not efficient and leaks resources by the end of request.
It is therefore recommended to minimize the usage of PHP callbacks.

README.md Outdated Show resolved Hide resolved
Comment on lines +82 to +88
$kvRead = new HostFunction("kv_read", [ExtismValType::I64], [ExtismValType::I64], function (CurrentPlugin $p, string $key) use (&$kvstore) {
return $kvstore[$key] ?? "\0\0\0\0";
});

$kvWrite = new HostFunction("kv_write", [ExtismValType::I64, ExtismValType::I64], [], function (string $key, string $value) use (&$kvstore) {
$kvstore[$key] = $value;
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhmd-azeez -- this is pretty neat! the ability to auto-cast (?) these types, e.g. (string $key, string $value) in the signature is impressive. Maybe it's just my lack of familiarity with PHP, but I think it would be really helpful to have some notes/docs on this in the source for this callback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I added parameter details to the README, not sure if that's what you meant. let me know if you think we need more info

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those work great - thank you!

return $p->write_block($value);
});

$kvWrite = new HostFunction("kv_write", [ExtismValType::I64, ExtismValType::I64], [], function (CurrentPlugin $p, int $keyPtr, int $valuePtr) use (&$kvstore) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(related to the previous comment)

I suppose the docs I'm looking for are primarily about how the transformation of the "higher level" types into these works. And, what are the limits? If my string for example, is actually bytes, is there any issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK there is no byte[] type in php, and string is used in its place in the standard library

Copy link
Contributor

@zshipko zshipko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dynamic typing is great! And the host function bindings look good too - I read through several times and didn't see any issues.

I think this is ready to merge once you figure out if you need to release the callback memory.

@mhmd-azeez
Copy link
Contributor Author

After a lot of experimenting, it seems like host functions leak memory, which is expected because it's a documented behavior of FFI + callbacks. So, I have decided to mark it as experimental for now, until we can find ways around this or maybe we get more user feedback.

However, the good news is, http servers like Apache are already designed to handle such scenarios fairly well, for example, let's consider this test:

for ($i = 0; $i < 10; $i++) {
    $kvstore = [];

    $kvRead = new HostFunction("kv_read", [ExtismValType::I64], [ExtismValType::I64], function (CurrentPlugin $p, string $key) use (&$kvstore) {
        return $kvstore[$key] ?? "\0\0\0\0";
    });

    $kvWrite = new HostFunction("kv_write", [ExtismValType::I64, ExtismValType::I64], [], function (string $key, string $value) use (&$kvstore) {
        $kvstore[$key] = $value;
    });

    $plugin = new Plugin($manifest, true, [$kvRead, $kvWrite]);
    $output = $plugin->call("count_vowels", "Hello World!");

    if ($i % 1 === 0) {
        echo "Iteration: $i => " . $lib->count . PHP_EOL;
    }
}

This is how Apache behaves under test:

php-sdk-memory-test.mp4

As for freeing memory,

Managed data lives together with the returned FFI\CData object, and is released when the last reference to that object is released by regular PHP reference counting or GC. Unmanaged data should be released by calling FFI::free(), when no longer needed.

-- PHP Docs

We are only using managed data, so we should be fine.

Copy link
Member

@nilslice nilslice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! Thank you

@mhmd-azeez mhmd-azeez merged commit 0a522eb into main Jan 29, 2024
2 checks passed
@mhmd-azeez mhmd-azeez deleted the feat/host-functions branch January 29, 2024 14:12
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.

Add support for Host Functions
4 participants