-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[OptionsResolver] refactoring/optimization #5073
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
Conversation
the normalization array is not needed as we can directly work with the normalizers array. this also reduces memory as we don't need to store normalizing closures that have been applied
Can you please include a performance measurement with before/after values? The methods that you changed here are partially executed a few thousand times a request, so performance is absolutely critical here (and was the reason for the code duplication). |
Before writing a performance test case I wanted to get some real usage data. So I counted to number of times the methods of the Options class are invoked for a big form of mine: Anyway, I'm pretty sure this PR is valid. I will try prove this with a performance measurement of the Options class standalone. |
Which is the very reason for their existence. If a lazy option wasn't evaluated for each different set of options, that would very much defeat its purpose. A possible improvement could be to remove the LazyOption class and to replace it by some smart logic in Options. I'm not sure whether this gains any performance though.
Given the above numbers of executions, improvements here have a big effect. |
I added a performance test. In terms of speed I could't determine any significant change (no wonder considering the tiny changes). But after my PR the memory usage for the test was reduced to 2/5. |
I tested your PR against Denis' form and there it decreases performance by 20ms. I don't like that. I agree that the property "normalization" can be removed, but "lazy" should stay in place, otherwise we are unnecessarily looping over all options everytime we are resolving. |
This is addressed by #5102 |
I'd like to know your performance testing methodology. How many samples do you have and what's the avarage and standard deviation? When I tested it, the varianz was too big to be statistically relevant. |
I can remove the looping over all options on each resolving and still remove the lazy array. |
I take a much more pragmatic approach to performance testing, which is browser testing with the above sample and comparing the before/after state of a change side by side. The deviation is usually around 5ms, so you can spot changes that have an impact >10ms quite well. Check out #5102, where lazy stays in place and the class LazyOption is removed. That PR both removes the memory problem you are talking about and at the same times improves performance for ~15ms. |
Browser testing with a deviation of 5ms? I don't think so. I have a minimum of 50ms. |
I'm glad to make you smile. You're right that a more solid, scientifically correct test bed would be nice™, but until now this was quick, sufficient and brought good results ;) |
see commits