Doctrine 2 - ORM
  1. Doctrine 2 - ORM
  2. DDC-217

Result cache should cache the SQL result and not the final objects

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0-ALPHA3
    • Fix Version/s: 2.2
    • Component/s: ORM
    • Security Level: All
    • Labels:
      None

      Description

      When fetching objects and using a result cache, it would probably be better to cache the SQL result array instead of the hydrated objects. That way, when grabbing from the cache managed entities can be returned. Also, caching is more efficient since an SQL result array does not need to be serialized/unserialized when storing in apc or memcached.

        Issue Links

          Activity

          Roman S. Borschel created issue -
          Roman S. Borschel made changes -
          Field Original Value New Value
          Priority Major [ 3 ] Minor [ 4 ]
          Hide
          Benjamin Eberlei added a comment -

          It also avoids unnecessary "proxy not initialized, can't serialize" exceptions.

          Show
          Benjamin Eberlei added a comment - It also avoids unnecessary "proxy not initialized, can't serialize" exceptions.
          Hide
          Jonathan H. Wage added a comment -

          If we make this change, the caching won't be "as" affective right since it will still have to go through the hydration process?

          Show
          Jonathan H. Wage added a comment - If we make this change, the caching won't be "as" affective right since it will still have to go through the hydration process?
          Hide
          Benjamin Eberlei added a comment -

          Correct, but its almost impossible to result cache entities because they are not allowed to have a single proxy entity at any depth of the object graph.

          Maybe we should introduce another notion of SqlResultCache? The ResultCache is good and effective for all Array and Scalar hydration modes, however the SQL Result caching is more useful for Object Hydration.

          Show
          Benjamin Eberlei added a comment - Correct, but its almost impossible to result cache entities because they are not allowed to have a single proxy entity at any depth of the object graph. Maybe we should introduce another notion of SqlResultCache? The ResultCache is good and effective for all Array and Scalar hydration modes, however the SQL Result caching is more useful for Object Hydration.
          Hide
          Roman S. Borschel added a comment -

          Exactly. Yes it means hydration is always run but you really cant speak of "effective" when caching large object result sets because serialization & especially unserialization which then happens on every request will be pretty slow.

          As Benjamin said, result caching is perfect for anything but objects so this ticket really only affects queries that use object hydration. Storing the raw sql result for these has the advantage that the objects retrieved from the cache are not detached and thus need not be merged and also you dont have issues with uninitialized proxies preventing serialization.

          Show
          Roman S. Borschel added a comment - Exactly. Yes it means hydration is always run but you really cant speak of "effective" when caching large object result sets because serialization & especially unserialization which then happens on every request will be pretty slow. As Benjamin said, result caching is perfect for anything but objects so this ticket really only affects queries that use object hydration. Storing the raw sql result for these has the advantage that the objects retrieved from the cache are not detached and thus need not be merged and also you dont have issues with uninitialized proxies preventing serialization.
          Hide
          Jonathan H. Wage added a comment -

          Is this also the cause of this issue? http://www.doctrine-project.org/jira/browse/DDC-446

          Since we're unserializing some objects, does Doctrine know about them?

          Show
          Jonathan H. Wage added a comment - Is this also the cause of this issue? http://www.doctrine-project.org/jira/browse/DDC-446 Since we're unserializing some objects, does Doctrine know about them?
          Hide
          Roman S. Borschel added a comment -

          Yes, as already said, objects coming from the result cache are currently DETACHED. Thus the EM does not care about them. This is fine if you just want to display them but if you want to modify them and persist new changes, you need to merge() the ones you want to become MANAGED again into the EM.

          Show
          Roman S. Borschel added a comment - Yes, as already said, objects coming from the result cache are currently DETACHED. Thus the EM does not care about them. This is fine if you just want to display them but if you want to modify them and persist new changes, you need to merge() the ones you want to become MANAGED again into the EM.
          Hide
          Roman S. Borschel added a comment -

          It should also be noticed that if we make this change and the cached objects are managed after grabbing from the cache this can have a negative effect on flushing performance, if you only want the cached objects for read-only/display purposes. Maybe we should still provide both options, like Benjamin said.

          Show
          Roman S. Borschel added a comment - It should also be noticed that if we make this change and the cached objects are managed after grabbing from the cache this can have a negative effect on flushing performance, if you only want the cached objects for read-only/display purposes. Maybe we should still provide both options, like Benjamin said.
          Roman S. Borschel made changes -
          Link This issue is referenced by DDC-446 [ DDC-446 ]
          Hide
          Roman S. Borschel added a comment -

          Moved to 2.1 for now.

          Show
          Roman S. Borschel added a comment - Moved to 2.1 for now.
          Roman S. Borschel made changes -
          Fix Version/s 2.1 [ 10022 ]
          Hide
          Daniel Cousineau added a comment -

          I would like to see the ability to cache pre-hydration as I have already run into this issue.

          Maybe the interface could be along similar syntaxes as turning on result caches

          $query->useResultCache(true)
                ->setResultCacheLifetime(3600)
                ->setResultCachePreHydration(true); //Result cache should work pre-hydration, false caches 
          

          However from my examinations the hydrators will have to be rewritten to accept an array instead of a pdo statement.

          That would be my vote. If you don't have the resources to work on the ticket I can tackle it.

          Show
          Daniel Cousineau added a comment - I would like to see the ability to cache pre-hydration as I have already run into this issue. Maybe the interface could be along similar syntaxes as turning on result caches $query->useResultCache( true ) ->setResultCacheLifetime(3600) ->setResultCachePreHydration( true ); //Result cache should work pre-hydration, false caches However from my examinations the hydrators will have to be rewritten to accept an array instead of a pdo statement. That would be my vote. If you don't have the resources to work on the ticket I can tackle it.
          Hide
          Benjamin Eberlei added a comment -

          rewriting to use an array is not an option. this would require to call ->fetchAll() which is not wanted for the Iterate Result feature.

          There has to be a better way to redefine the method signatures to allow both use-cases.

          Show
          Benjamin Eberlei added a comment - rewriting to use an array is not an option. this would require to call ->fetchAll() which is not wanted for the Iterate Result feature. There has to be a better way to redefine the method signatures to allow both use-cases.
          Hide
          Benjamin Eberlei added a comment -

          Ah btw, any help is appreciated. Just Fork doctrine2 over at Github and develop this feature in a new branch (git checkout -b DDC-217) and link it here or issue a pull request on Github.

          Show
          Benjamin Eberlei added a comment - Ah btw, any help is appreciated. Just Fork doctrine2 over at Github and develop this feature in a new branch (git checkout -b DDC-217 ) and link it here or issue a pull request on Github.
          Hide
          Daniel Cousineau added a comment -

          Forked here: https://github.com/dcousineau/doctrine2 though probably should pull it out into a local branch.

          I went ahead and went down that route of altering hydrators to accept an array as well as a statement. This way uncached queries will work with the statement as it used to, but can also hydrate from an array.

          As for the iterable hydrator you have a point but in this case you really can't cache it period, be it pre or post hydration (since to cache we have to serialize ALL the information).

          What I have up is working(ish with the caveat that I backported the changes to an earlier tagged release for our application and tested using that), take a look and see if we like or don't like how I did it.

          You'll notice I did split up _doExecute() for the query objects, because the DQL query object does so much analysis in the _doExecute() method which produces and attaches information (namely the result set mapping) that is depended on by the hydration process, I split it up into a "doPreExecute" function so that it can be called separately without actually executing the query.

          Show
          Daniel Cousineau added a comment - Forked here: https://github.com/dcousineau/doctrine2 though probably should pull it out into a local branch. I went ahead and went down that route of altering hydrators to accept an array as well as a statement. This way uncached queries will work with the statement as it used to, but can also hydrate from an array. As for the iterable hydrator you have a point but in this case you really can't cache it period, be it pre or post hydration (since to cache we have to serialize ALL the information). What I have up is working(ish with the caveat that I backported the changes to an earlier tagged release for our application and tested using that), take a look and see if we like or don't like how I did it. You'll notice I did split up _doExecute() for the query objects, because the DQL query object does so much analysis in the _doExecute() method which produces and attaches information (namely the result set mapping) that is depended on by the hydration process, I split it up into a "doPreExecute" function so that it can be called separately without actually executing the query.
          Hide
          Benjamin Eberlei added a comment -

          Yes i know that the serialize thing won't work with the Iterator. My point was that the Iterator USES the Hydrator code and it should still support that use-case.

          See the IterableResult class and make sure that its still working after your refactoring.

          I don't like the instanceof check then iterate vs the forach over the array. I would rather find a solutio nthat works for both, but i cant find one now that doesnt involve findAll() (which is bad).

          Show
          Benjamin Eberlei added a comment - Yes i know that the serialize thing won't work with the Iterator. My point was that the Iterator USES the Hydrator code and it should still support that use-case. See the IterableResult class and make sure that its still working after your refactoring. I don't like the instanceof check then iterate vs the forach over the array. I would rather find a solutio nthat works for both, but i cant find one now that doesnt involve findAll() (which is bad).
          Hide
          Daniel Cousineau added a comment -

          I'll double check against the IterableResult and make sure it's compatible.

          Beyond that, I don't know what I could other than the instance of check. I could extend the statement object and create a statement that takes a flat array so all the code remains the same (i'm passing a statement).

          Show
          Daniel Cousineau added a comment - I'll double check against the IterableResult and make sure it's compatible. Beyond that, I don't know what I could other than the instance of check. I could extend the statement object and create a statement that takes a flat array so all the code remains the same (i'm passing a statement).
          Hide
          Benjamin Eberlei added a comment -

          Assigning Guilherme

          Show
          Benjamin Eberlei added a comment - Assigning Guilherme
          Benjamin Eberlei made changes -
          Assignee Roman S. Borschel [ romanb ] Guilherme Blanco [ guilhermeblanco ]
          Hide
          Przemek Sobstel added a comment - - edited

          I've made my own implementation for this:
          https://github.com/sobstel/doctrine2/commit/f583625f791da22dec77dd7d6c83b813f0bcbdaa

          There are no changes in specific hydrators and there is no fetchAll(). I've added 2 wrappers: one for cached result array, and one for uncached pdo statement. The former (CachedResultStatement) is actually ArrayIterator which just implements used methods (fetch & closeCursor => also specified in ResultStatementInterface). The latter is decorator for stmt, which just collects results on each call of fetch() (results are later available by calling getResult), and apart from this all calls are delegated to actual stmt object.

          I've used (copied) following changes from Daniel https://github.com/dcousineau/doctrine2/commit/c23b5a902da79ffc472c0769b9258775baf3189e

          Btw, what is your reasoning for not using fetchAll() anywhere?

          Show
          Przemek Sobstel added a comment - - edited I've made my own implementation for this: https://github.com/sobstel/doctrine2/commit/f583625f791da22dec77dd7d6c83b813f0bcbdaa There are no changes in specific hydrators and there is no fetchAll(). I've added 2 wrappers: one for cached result array, and one for uncached pdo statement. The former (CachedResultStatement) is actually ArrayIterator which just implements used methods (fetch & closeCursor => also specified in ResultStatementInterface). The latter is decorator for stmt, which just collects results on each call of fetch() (results are later available by calling getResult), and apart from this all calls are delegated to actual stmt object. I've used (copied) following changes from Daniel https://github.com/dcousineau/doctrine2/commit/c23b5a902da79ffc472c0769b9258775baf3189e Btw, what is your reasoning for not using fetchAll() anywhere?
          Hide
          Reio Piller added a comment - - edited

          maybe a dumb question but is it normal that result cache fetched entities associations are set to blank objects?

          Show
          Reio Piller added a comment - - edited maybe a dumb question but is it normal that result cache fetched entities associations are set to blank objects?
          Benjamin Eberlei made changes -
          Fix Version/s 2.x [ 10090 ]
          Fix Version/s 2.1 [ 10022 ]
          Hide
          Bogdan Albei added a comment -

          I've sent a pull request for the fix(https://github.com/bogdanalbei/doctrine2/commit/00d1aa2192b86e51872fbc833b2436354f6fe162) . The changes were tested under live conditions sice v2.0.

          Show
          Bogdan Albei added a comment - I've sent a pull request for the fix( https://github.com/bogdanalbei/doctrine2/commit/00d1aa2192b86e51872fbc833b2436354f6fe162 ) . The changes were tested under live conditions sice v2.0.
          Benjamin Eberlei made changes -
          Link This issue is duplicated by DDC-1279 [ DDC-1279 ]
          Hide
          Benjamin Eberlei added a comment -

          Workaround for this issue: https://gist.github.com/57e6b4deea566baac053

          Also: This will be fixed in 2.2.

          Show
          Benjamin Eberlei added a comment - Workaround for this issue: https://gist.github.com/57e6b4deea566baac053 Also: This will be fixed in 2.2.
          Benjamin Eberlei made changes -
          Assignee Guilherme Blanco [ guilhermeblanco ] Benjamin Eberlei [ beberlei ]
          Benjamin Eberlei made changes -
          Fix Version/s 2.2-DEV [ 10157 ]
          Fix Version/s 2.x [ 10090 ]
          Hide
          Benjamin Eberlei added a comment -

          First step of this hit DBAL today https://github.com/doctrine/dbal/pull/69

          Show
          Benjamin Eberlei added a comment - First step of this hit DBAL today https://github.com/doctrine/dbal/pull/69
          Hide
          Benjamin Eberlei added a comment -

          DBAL code was heavily refactored and merged, the ORM PR is now open for discussion:

          https://github.com/doctrine/doctrine2/pull/172

          Show
          Benjamin Eberlei added a comment - DBAL code was heavily refactored and merged, the ORM PR is now open for discussion: https://github.com/doctrine/doctrine2/pull/172
          Hide
          Benjamin Eberlei added a comment -

          Merged into master

          Show
          Benjamin Eberlei added a comment - Merged into master
          Benjamin Eberlei made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Benjamin Eberlei added a comment -

          Related Pull Request was closed: https://github.com/doctrine/doctrine2/pull/87

          Show
          Benjamin Eberlei added a comment - Related Pull Request was closed: https://github.com/doctrine/doctrine2/pull/87
          Benjamin Eberlei made changes -
          Workflow jira [ 10634 ] jira-feedback [ 14207 ]
          Benjamin Eberlei made changes -
          Workflow jira-feedback [ 14207 ] jira-feedback2 [ 16071 ]
          Benjamin Eberlei made changes -
          Workflow jira-feedback2 [ 16071 ] jira-feedback3 [ 18324 ]

          This list may be incomplete, as errors occurred whilst retrieving source from linked applications:

          • Request to http://www.doctrine-project.org/fisheye/ failed: Error in remote call to 'FishEye 0 (http://www.doctrine-project.org/fisheye/)' (http://www.doctrine-project.org/fisheye) [AbstractRestCommand{path='/rest-service-fe/search-v1/crossRepositoryQuery', params={query=DDC-217, expand=changesets[0:20].revisions[0:29],reviews}, methodType=GET}] : Received status code 503 (Service Temporarily Unavailable)

            People

            • Assignee:
              Benjamin Eberlei
              Reporter:
              Roman S. Borschel
            • Votes:
              5 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: