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

[9.x] Add ability to mark a class as singleton with interface #41485

Closed
wants to merge 1 commit into from

Conversation

pionl
Copy link
Contributor

@pionl pionl commented Mar 15, 2022

Add ability to mark a class as singleton with interface instead of using service provider and.

Why?

When we create a lot of services / actions and we are heavily using dependency injection it is better to mark the classes as singleton to speed things up.

It takes some time to register all actions / services in service provider and the file is getting bigger. This would make marking singleton easy to write and even enables to find which classes are singletons via using IDE and find usages.

Checklist

  • Unit test
  • Tested on real app - I have not found any performance issues (no notable difference in ms).
  • Does it math Laravel ecosystem? I think yes, there is already a lot of "magic" using interfaces
class SharedStub implements IsSingleton {
    //
}

Thank you for your time

… of service->register

class SharedStub implements IsSingleton {
    //
}
@driesvints
Copy link
Member

It takes some time to register all actions / services in service provider and the file is getting bigger.

Really? What use case do you have to register so much services that it slows your app down? I don't see how this is possible.

This would make marking singleton easy to write and even enables to find which classes are singletons via using IDE and find usages.

This isn't an exact truth tbh. You could still register the classes as non-singletons. Tbh, you can already create your own interface in userland if you wanted to to achieve this.

@pionl
Copy link
Contributor Author

pionl commented Mar 15, 2022

  1. It is some time ago, but in quite large code base with heavy dependency injection and using events system the app, while responding on events, had some performance issues. There were bigger loops (not so big) that would trigger events and Laravel would create listeners. We did a profile of the application and found most of the bottleneck was in make method of container. We did use singleton on the services that listener used and this resolved the issues.
  2. I've no idea how you mean this. Without extending and changing container implementation.

Of course I can do it in service provider, but if I want to move services / actions that are heavily used then I want to mark then as singleton to prevent issues above :) There is minimal memory cost on this.

If you do not like it, we can close the PR, I've no problem with that. It was an idea how to simplify my life (and maybe others).

Don't want to waste your time if you don't see it matching teams vision 👍

Thanks

@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

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

3 participants