-
-
Notifications
You must be signed in to change notification settings - Fork 102
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
Make it faster #56
Comments
A graph to give an idea of the performance impact: https://blackfire.io/profiles/b4b98b5d-f850-4853-b549-6ebe326a3451/graph For ~150K it took ~10s for my data structures (which are not very big). |
That would mean that dynamically added properties would not be cloned? |
There is cases, like in PhpUnit where this behaviour would not be desired. However in the case of alice of example, saving from |
Two things that could be done:
Last be not least, performances should be monitored. Having a benchmark with a proper scenario would be cool. |
@theofidry have a look at https://github.com/mnapoli/ReflectionBenchmark It might be worth running it again with PHP 7 though since performance has changed a lot. |
Exactly what I was looking for :) Why not putting it in this project though? |
What do you mean putting it in this project? |
By having a similar benchmark in this project* |
Soz a bit tired lately ;) |
Oh OK, but it already exists there I don't see a reason to duplicate it (and it's a generic benchmark so…). Or maybe you meant to add a benchmark of DeepCopy, in that case 👍 Anyway feel free to work on all that, that package is not my priority at the moment. I don't have admin access to the repository (this is the organisation of a former employer of mine) but I'm getting that sorted out soon, I'll add you as collaborator as soon as I can, that will be much more practical for you. |
Yeah that's what I was thinking of. It would be great if you could retrieve access to it. I have almost no time for OSS lately but I'll need to have a break and make some progress soon. I'll get this [handle of the issues I posted] sorted then. |
@theofidry you should now have write access to the repository! |
👌 |
This helps: #107 |
Could be, did you notice any performance improvements with it? |
Without benchmarks any perception is biased, but for sure removing this: |
Last time I looked, I couldn't find any way to really squeeze much more perf. This is mainly due to the fact that
DeepCopy
deep copy an object. As such, you cannot assume the properties of that object as values can be dynamically added. The result isDeepCopy
must rely on a brand new instance ofReflectionObject
each time, even if you get two objects of the same class.A way to change that would maybe to add a mode to clone class instances instead, i.e. to rely on
ReflectionClass
and cache it somehow.But to be honest this is just an idea like that. I'm actually not sure if it's really worth it, maybe it would be better to start by having a benchmark and see if the perf gain is worth it.
The text was updated successfully, but these errors were encountered: