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

Various performance improvements (cache entities, entity and property de... #228

Merged
merged 2 commits into from Jan 23, 2015

Conversation

andig
Copy link
Contributor

@andig andig commented Jan 21, 2015

...finitions, use Iterators instead of callbacks)

… definitions, use Iterators instead of callbacks)
@jahir
Copy link
Member

jahir commented Jan 21, 2015

tnx. Only difference to #215 is DataController line 129, right?
AFAICS this disables caching for POST entirely, which is a little pity, because there are much more POSTs than GETs (at least on my system), so the caching primarily helps the frontend.

I'm not sure why caching works for get but not for add. The add method wants a channel and the get an entity, but that shouldn't be the problem (as we EntityController::getSingleResult stores in the cache what it returns). If it's easy to make it work for both, could you do that? The class looks kind of scary, anyway...

And could you add some comments to the new option in volkszaehler.conf.php, maybe with some description what is cached in which case.

@andig
Copy link
Contributor Author

andig commented Jan 21, 2015

AFAICS this disables caching for POST entirely,

Partially true. Disabled for POSTs not using JSON (i.e. via GET).

which is a little pity, because there are much more POSTs

True but not a problem the user typically encounters.

To solve, either:

  1. change DataController->add to always use SQL instead of ORM (as the JSON solution is already doing) or
  2. make clients use POST requests

I'd he happy to add support for 1) but it feels like a move in the wrong direction. Would again be faster though.

@jahir
Copy link
Member

jahir commented Jan 21, 2015

Partially true. Disabled for POSTs not using JSON (i.e. via GET).

Well, my clients use POST but with URL parameters :)

  1. change DataController->add to always use SQL instead of ORM (as the JSON solution is already
    ...
    I'd he happy to add support for 1) but it feels like a move in the wrong direction. Would again be faster though.

I guess we should decide at one point if we want to keep the ORM (and use it everywhere) or get completely rid of it...
OTOH, I care more about about functional uniformity, e.g. the add method should not behave differently between GET &action=add and POST

@andig
Copy link
Contributor Author

andig commented Jan 21, 2015

Well, my clients use POST but with URL parameters :)

Thats not the problem.

OTOH, I care more about about functional uniformity, e.g. the add method should not behave differently between GET &action=add and POST

It does not. The distinction is JSON in POST body or not. Reason is that only POST body can carry multiple timestamp/value pairs. It's easy enough to change the other case to SQL as well.

I guess we should decide at one point if we want to keep the ORM (and use it everywhere) or get completely rid of it...

Probably not as the whole entity management is quite complex and Doctrine has clear advantages. Why not use pure SQL for the parts where it matters (e.g. Aggregation)?

@andig
Copy link
Contributor Author

andig commented Jan 22, 2015

@jahir Updated DataController to always use SQL- we gain consistency and performance and loose some old code.

This should be the version we commit.

@andig andig force-pushed the cache-entities branch 2 times, most recently from 90e0d9c to ca4847a Compare January 22, 2015 16:36
jahir pushed a commit that referenced this pull request Jan 23, 2015
Various performance improvements (cache entities, entity and properties)
@jahir jahir merged commit e5d8c0b into volkszaehler:master Jan 23, 2015
@jahir
Copy link
Member

jahir commented Jan 23, 2015

nicely done, thanks! :)

@andig andig deleted the cache-entities branch February 21, 2015 15:56
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

2 participants