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(bindings): add bindings for cached function calls #315

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Xenira
Copy link
Collaborator

@Xenira Xenira commented May 25, 2024

For my extension I need to perform a lot of callbacks.

Using fci (fcall_info) and fcc (fcall_info_cache) yielded ~10% performance improvement.

If I find the time Ill port it into this lib. For now I just added the bindings to be able to use them in my code directly.

You might need to check the docrs_bindings.rs as it seems there are more changes then there should be (maybe php version?). Mainly u8 -> u16

@Xenira
Copy link
Collaborator Author

Xenira commented May 25, 2024

https://github.com/davidcole1340/ext-php-rs/actions/runs/9237212872/job/25414068937?pr=315
This a macos problem? Seems like there are some macos steps in the build.yml.

@Xenira
Copy link
Collaborator Author

Xenira commented May 25, 2024

Maybe specifying the arch helps. Just trying stuff though.

@Xenira
Copy link
Collaborator Author

Xenira commented May 25, 2024

Ummm, arch seems to be only supported from v2. Updated the action.

@Xenira
Copy link
Collaborator Author

Xenira commented May 27, 2024

Seems to still have x86_64 clang. install-llvm-action seems to be using cache. Can we try to clear the cache?

@Xenira
Copy link
Collaborator Author

Xenira commented May 31, 2024

How do we proceed with the pipeline issue?

@ptondereau
Copy link
Collaborator

Hey @Xenira
Thank you for your patience, I've cleaned the cache and re-runned the CI and now, you have another error.

@ptondereau
Copy link
Collaborator

ptondereau commented May 31, 2024

BTW, if you want to add more bindings, you need to have the most up-to-date stable version of PHP (8.3.7 for now) with debug mode (it exposes more symbols for bindgen)

@Xenira
Copy link
Collaborator Author

Xenira commented Jun 1, 2024

Hey @Xenira Thank you for your patience, I've cleaned the cache and re-runned the CI and now, you have another error.

Thanks! Did a quick google search and it seems to be a problem with dylib and the mac libc https://discourse.llvm.org/t/apples-libc-now-provides-std-type-descriptor-t-functionality-not-found-in-upstream-libc/73881

Ill try to set up my own pipeline to iterate faster on this.

BTW, if you want to add more bindings, you need to have the most up-to-date stable version of PHP (8.3.7 for now) with debug mode (it exposes more symbols for bindgen)

I'm on 8.3.7, not sure about debug though. Is that related to the differences in docrs_bindgen.rs? Because the changes itself worked fine and I was able to use the new bindings.

@Xenira
Copy link
Collaborator Author

Xenira commented Aug 5, 2024

Hey @ptondereau,

Finally got to test it.

This seems to be caused by clang / llvm 15 and 17. Supposedly it should work with 18, but there is no macos build yet.

I managed to get it to work by skipping the clang / llvm setup entirely for macos https://github.com/Xenira/ext-php-rs-pipeline-debug/actions/runs/10254367953/job/28368987959

How should we proceed with this? Easiest would be to skip the llvm setup for the macos runs.

@ptondereau
Copy link
Collaborator

Hey @ptondereau,

Finally got to test it.

This seems to be caused by clang / llvm 15 and 17. Supposedly it should work with 18, but there is no macos build yet.

I managed to get it to work by skipping the clang / llvm setup entirely for macos https://github.com/Xenira/ext-php-rs-pipeline-debug/actions/runs/10254367953/job/28368987959

How should we proceed with this? Easiest would be to skip the llvm setup for the macos runs.

Yes I've tried to get a full build on macos too but as you said, 18 is not available...
I'm not a huge fan about skipping MacOS for the moment but maybe @danog has some plan?

@Xenira
Copy link
Collaborator Author

Xenira commented Aug 6, 2024

Wouldn't skip macos entirely. Just leaving out the setup steps makes macos build again https://github.com/Xenira/ext-php-rs-pipeline-debug/blob/0298ee6a016d1ddcb7178e904601485731746723/.github/workflows/build.yml#L64

@ptondereau
Copy link
Collaborator

Wouldn't skip macos entirely. Just leaving out the setup steps makes macos build again https://github.com/Xenira/ext-php-rs-pipeline-debug/blob/0298ee6a016d1ddcb7178e904601485731746723/.github/workflows/build.yml#L64

Ah ok! I'm ok about that, just leave a comment about the why and it should be good for me

@Xenira
Copy link
Collaborator Author

Xenira commented Aug 6, 2024

Well, now clippy complains all over the place. Will create a seperate mr to get pipeline fixed.

@Xenira Xenira mentioned this pull request Aug 7, 2024
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.

2 participants