Skip to content
This repository has been archived by the owner on Dec 2, 2021. It is now read-only.

Add lazy Renderer service to speedup page load time #66

Merged
merged 7 commits into from Aug 17, 2016

Conversation

umpirsky
Copy link
Contributor

Fixes #59.

@coveralls
Copy link

coveralls commented Aug 17, 2016

Coverage Status

Coverage decreased (-1.9%) to 98.091% when pulling 41ad4f8 on ChessCom:fix/issue-59 into dd8ea15 on scheb:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.9%) to 98.091% when pulling 41ad4f8 on ChessCom:fix/issue-59 into dd8ea15 on scheb:master.

@coveralls
Copy link

coveralls commented Aug 17, 2016

Coverage Status

Coverage decreased (-1.9%) to 98.091% when pulling 41ad4f8 on ChessCom:fix/issue-59 into dd8ea15 on scheb:master.

1 similar comment
@coveralls
Copy link

coveralls commented Aug 17, 2016

Coverage Status

Coverage decreased (-1.9%) to 98.091% when pulling 41ad4f8 on ChessCom:fix/issue-59 into dd8ea15 on scheb:master.

@coveralls
Copy link

coveralls commented Aug 17, 2016

Coverage Status

Coverage decreased (-0.5%) to 99.523% when pulling 5cebf92 on ChessCom:fix/issue-59 into dd8ea15 on scheb:master.

@coveralls
Copy link

coveralls commented Aug 17, 2016

Coverage Status

Coverage decreased (-0.5%) to 99.523% when pulling 4c35b23 on ChessCom:fix/issue-59 into dd8ea15 on scheb:master.

@coveralls
Copy link

coveralls commented Aug 17, 2016

Coverage Status

Coverage decreased (-0.5%) to 99.523% when pulling e7f0238 on ChessCom:fix/issue-59 into dd8ea15 on scheb:master.

@coveralls
Copy link

coveralls commented Aug 17, 2016

Coverage Status

Coverage decreased (-0.5%) to 99.523% when pulling ea91992 on ChessCom:fix/issue-59 into dd8ea15 on scheb:master.

@coveralls
Copy link

coveralls commented Aug 17, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling eeba4a3 on ChessCom:fix/issue-59 into dd8ea15 on scheb:master.

@umpirsky umpirsky changed the title [WIP] Add lazy Renderer service to speedup page load time Add lazy Renderer service to speedup page load time Aug 17, 2016
@umpirsky umpirsky mentioned this pull request Aug 17, 2016
@umpirsky
Copy link
Contributor Author

@scheb Can you review please?

@coveralls
Copy link

coveralls commented Aug 17, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 23f17a2 on ChessCom:fix/issue-59 into dd8ea15 on scheb:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 23f17a2 on ChessCom:fix/issue-59 into dd8ea15 on scheb:master.

@scheb
Copy link
Owner

scheb commented Aug 17, 2016

Looks good, thanks!

@scheb scheb merged commit e7f9422 into scheb:master Aug 17, 2016
@zerkms
Copy link
Contributor

zerkms commented Aug 17, 2016

So what was the "problem"? The templating service?

I'm not sure I understand the roots of the problem just looking at this code.

@zerkms
Copy link
Contributor

zerkms commented Aug 17, 2016

Btw, why is it the doctrine dependency now?

@umpirsky
Copy link
Contributor Author

@scheb Thanks for blazing fast reaction! 👍

@zerkms Yes. It's not a dependency, it's dev dependency, we need it for tests.

@umpirsky umpirsky deleted the fix/issue-59 branch August 18, 2016 07:52
@zerkms
Copy link
Contributor

zerkms commented Aug 18, 2016

@umpirsky but what about some explanation? Was it templating? If so - how your app is structured so that it does not use templating service? Since all mine always use it even for API (fosrestbundle)

@umpirsky
Copy link
Contributor Author

@zerkms Yes, they use it, I just wanted to remove this bundle services from the slowest list. Plus, this seems like a good decoupling.

@zerkms
Copy link
Contributor

zerkms commented Aug 18, 2016

Yes, they use it, I just wanted to remove this bundle services from the slowest list

If the app uses it anyway - this change is harmful: it increases CPU + memory load

@scheb so, the change you accepted made the bundle slower and more memory hungry in favour of @umpirsky performance statistics to look "better" ("better" here in quotes because it only spread the slow path into multiple paths, but in fact does not improve anything). "Looks good, thanks!" --- does it really look that good to you? :-S

Plus, this seems like a good decoupling.

Cannot agree with it.

Colleagues, I understand that you like nice graphs, but this change makes totally no sense.

@umpirsky
Copy link
Contributor Author

@zerkms After this you can override Renderer and use other template engine then Twig.

Adding 2 services is minor since typcal Symfony app can have more then thousands of services.

@zerkms
Copy link
Contributor

zerkms commented Aug 18, 2016

Adding 2 services is minor since typcal Symfony app can have more then thousands of services.

That, plus few extra lazy wrappers. All of that only for someone's graphs to look better.

After this you can override Renderer and use other template engine then Twig.

If only it was a problem.

I'm off anyway - I shared my thoughts, but you apparently value your performance metrics over actual performance. So, whatever.

@umpirsky
Copy link
Contributor Author

@zerkms I appreciate your opinion, but this is open source project and there is many ways people use this software. We are putting efforts to make it as flexible as possible with performance in mind of course. I still think that number of services is not a bottleneck.

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

Successfully merging this pull request may close these issues.

None yet

4 participants