Skip to content

Conversation

@zeriyoshi
Copy link
Contributor

Perhaps they can be handled by a common object handlers.

@TimWolla
Copy link
Member

TimWolla commented Aug 5, 2024

What are you trying to achieve with this change? Is this intended to improve performance? Memory Usage? Is this just intended as a code clean?

@zeriyoshi
Copy link
Contributor Author

@TimWolla
Sorry, my apologies. It is intended to reduce memory usage, albeit slightly, and clean up the code.

@zeriyoshi zeriyoshi force-pushed the random_common_handlers branch from 70ef769 to 4b6aba7 Compare August 15, 2024 02:41
@zeriyoshi
Copy link
Contributor Author

@TimWolla
Hello. I created this minor PR as a form of rehabilitation, but should we merge this into master in the future?
Since it involves ABI changes, I think we should merge it after the PHP-8.4 branch is created.

If you don't see any particular benefits, I'm thinking of closing it :)

@TimWolla
Copy link
Member

@zeriyoshi Sorry for the delay. I was busy in wrapping things up before the feature freeze and then I was on vacation.

I don't see a value-add in this change. It provides only a very small benefit in memory usage (if at all), but adds another layer of indirection that is not present in any other extension as far as I can tell. Instead of having all the object handlers clearly stated in MINIT one needs to carefully look them up. Also any future changes to them for one engine or any newly introduced engine would possibly require splitting them again.

@zeriyoshi
Copy link
Contributor Author

@TimWolla
Indeed. This is a kind of unnecessary optimization.
I'll create another PR if I find a different area that needs improvement! Thanks for reviewing!

@zeriyoshi zeriyoshi closed this Sep 2, 2024
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.

2 participants