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

Reintroduce the ext-eio adapter #112

Merged

Conversation

WyriHaximus
Copy link
Member

During the initial rewrite this adapter was removed to make the rewrite easier. Since that is now in adding the ext-eio adapter will provide several high performance adapters.

@WyriHaximus WyriHaximus added this to the v0.2.0 milestone Aug 26, 2022
@WyriHaximus WyriHaximus force-pushed the 0.2.x-reointroduce-eio-adapter branch 5 times, most recently from 7c14347 to b325284 Compare August 26, 2022 17:20
@WyriHaximus WyriHaximus force-pushed the 0.2.x-reointroduce-eio-adapter branch 2 times, most recently from 75f631c to 6e91496 Compare November 8, 2022 12:57
@WyriHaximus WyriHaximus marked this pull request as ready for review November 8, 2022 13:02
WyriHaximus added a commit to WyriHaximus-labs/event-loop that referenced this pull request Nov 8, 2022
This ties in directly with reactphp/filesystem#112 where if both ext-eio and ext-uv are present ext-uv interferes with ext-eio. By putting it lower in the factory construction method it will favor ext-eio over ext-uv for filesystem I/O is there is also a another event loop extension present.
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

@WyriHaximus Great progress! I've added some questions below, but like the direction overall!

src/Eio/Poll.php Outdated Show resolved Hide resolved
src/Eio/Poll.php Outdated Show resolved Hide resolved
src/Factory.php Show resolved Hide resolved
@WyriHaximus WyriHaximus force-pushed the 0.2.x-reointroduce-eio-adapter branch 4 times, most recently from bea9816 to f683982 Compare November 12, 2022 06:01
@WyriHaximus WyriHaximus force-pushed the 0.2.x-reointroduce-eio-adapter branch 4 times, most recently from 913b2fa to d7085d0 Compare December 8, 2022 07:21
@WyriHaximus
Copy link
Member Author

@clue updated the PR with findings regarding ext-eio and ext-uv, managed to resolve the issues, details in review thread

@WyriHaximus WyriHaximus requested a review from clue December 9, 2022 08:39
src/Eio/File.php Outdated Show resolved Hide resolved
src/Eio/File.php Outdated Show resolved Hide resolved
@lucasnetau
Copy link

Does the new adapter handle when eio_open returns false (or any other call, but eio_open appears to be the most common)?, it's undocumented but if you look into the PECL library you can see the error path that RETURN_FALSE instead of -1

See #43 was the reason we had to move to the child process for file operations

@WyriHaximus
Copy link
Member Author

Does the new adapter handle when eio_open returns false (or any other call, but eio_open appears to be the most common)?, it's undocumented but if you look into the PECL library you can see the error path that RETURN_FALSE instead of -1

As in it returns false, and doesn't call the handler you give it. Or it calls with handler with a false?

See #43 was the reason we had to move to the child process for file operations

Will have a thorough look again at #43, but during development, I haven't run into those issues for 0.2. 0.2 is aimed a keeping things a lot simpler and avoiding making the same mistakes I made in 0.1.

@WyriHaximus WyriHaximus force-pushed the 0.2.x-reointroduce-eio-adapter branch from c6187f8 to f380a64 Compare April 24, 2023 13:31
During the initial rewrite this adapter was removed to make the rewrite easier. Since that is now in adding the ext-eio adapter will provide several high performance adapters.
@WyriHaximus WyriHaximus force-pushed the 0.2.x-reointroduce-eio-adapter branch from f380a64 to e469054 Compare April 24, 2023 15:02
@clue
Copy link
Member

clue commented May 15, 2023

@WyriHaximus I've noticed the new eio related classes don't currently have 100% code coverage, is this something you plan to look into? Otherwise no objects to go ahead with this from my end 👍

@WyriHaximus
Copy link
Member Author

@clue yes, in the next PR I'll address several code coverage issues across the board by covering more situations such as partial reads.

@clue
Copy link
Member

clue commented May 30, 2023

Personally, I'd like to see better code coverage for this new adapter to make sure we're not introducing any regressions before moving forward with this. For instance, the stat syscall in getContents() as discussed above is unneeded and most library calls currently have limited error checks – but the same currently applies to all others adapters as well. With this in mind, I don't want to block moving forward with this PR and agree that these are things that can be addressed in follow-up PRs 👍

@WyriHaximus Let's merge this as is? :shipit:

@WyriHaximus
Copy link
Member Author

@clue Works for me, that is exactly what I'll be addressing in the follow up PR

@WyriHaximus WyriHaximus merged commit ca5e06f into reactphp:0.2.x May 30, 2023
@WyriHaximus WyriHaximus deleted the 0.2.x-reointroduce-eio-adapter branch May 30, 2023 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants