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

Make it faster #56

Open
theofidry opened this issue Oct 31, 2016 · 16 comments
Open

Make it faster #56

theofidry opened this issue Oct 31, 2016 · 16 comments
Milestone

Comments

@theofidry
Copy link
Collaborator

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 is DeepCopy must rely on a brand new instance of ReflectionObject 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.

@theofidry
Copy link
Collaborator Author

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).

@mnapoli
Copy link
Member

mnapoli commented Oct 31, 2016

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.

That would mean that dynamically added properties would not be cloned?

@theofidry
Copy link
Collaborator Author

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 stdClass and very rare cases (which already requires a lot of custom work), this behaviour is not needed.

@theofidry
Copy link
Collaborator Author

Two things that could be done:

  • Review where we could avoid unnecessary reflection
  • Maybe we could experiment with the closure scopes instead of reflection. The result is not guaranteed as with reflection you know how to access to a value. With the closure you do not hence you may have some error handling to do to properly retrieve a value.
  • Migrate to php7
  • Have a mode where you rely on the reflection class instead of reflection object. Should be configurable as it means dynamic properties for example won't be cloned anymore.
  • Maybe check Symfony PropertyAcccessor, IIRC there is some smart things done to retrieve the structure of a PHP object which could give some hints

Last be not least, performances should be monitored. Having a benchmark with a proper scenario would be cool.

@mnapoli
Copy link
Member

mnapoli commented Apr 29, 2017

@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.

@theofidry
Copy link
Collaborator Author

Exactly what I was looking for :) Why not putting it in this project though?

@mnapoli
Copy link
Member

mnapoli commented Apr 29, 2017

What do you mean putting it in this project?

@theofidry
Copy link
Collaborator Author

By having a similar benchmark in this project*

@theofidry
Copy link
Collaborator Author

Soz a bit tired lately ;)

@mnapoli
Copy link
Member

mnapoli commented Apr 29, 2017

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.

@theofidry
Copy link
Collaborator Author

Or maybe you meant to add a benchmark of DeepCopy, in that case

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.

@mnapoli
Copy link
Member

mnapoli commented May 1, 2017

@theofidry you should now have write access to the repository!

@theofidry
Copy link
Collaborator Author

👌

@theofidry theofidry added this to the 2.0 milestone Oct 15, 2017
@Slamdunk
Copy link
Contributor

This helps: #107

@theofidry
Copy link
Collaborator Author

Could be, did you notice any performance improvements with it?

@Slamdunk
Copy link
Contributor

Slamdunk commented May 31, 2018

Without benchmarks any perception is biased, but for sure removing this:
https://github.com/myclabs/DeepCopy/pull/107/files#diff-b80ac3509b6e72486b21cddfd9d9332aL58-L77
helped

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants