Skip to content

Fix(Hydrator): disabling identity map now disables the identity map#131

Merged
vgreb merged 2 commits intoccmbenchmark:masterfrom
xavierleune:fix/identity-map
Dec 16, 2025
Merged

Fix(Hydrator): disabling identity map now disables the identity map#131
vgreb merged 2 commits intoccmbenchmark:masterfrom
xavierleune:fix/identity-map

Conversation

@xavierleune
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Licence Apache-2.0
Fixed tickets N/A

Noticed today with a big batch of reads, when trying to optimize the memory consumption, that the identity map was not properly disabled. This PR fixes that.

@xavierleune xavierleune requested a review from vgreb as a code owner December 16, 2025 11:04
@vgreb
Copy link
Contributor

vgreb commented Dec 16, 2025

Wow ! Good Catch ! Does this bug exists since IdentityMap was introduced ?

@xavierleune
Copy link
Contributor Author

maybe I should give second thoughts on this one. This change seems really needed but I should probably check there is no unintended side effects more carefully. Seems to be there for a while now, really don't think it's on purpose but these lines makes me doubt: https://github.com/ccmbenchmark/ting/pull/131/changes#diff-1a1e1cbffbe64904034ef60574a22c8b697439bdb644371209c4c7c2a21c5093L337-L339

if (isset($this->references[$ref]) === true) {

      // This entity was already created and stored into references
      if ($this->identityMap === false) {
          // If identityMap is disabled, it clones the object and reset UUID
          $result[$column['table']] = clone $this->references[$ref];
      } else {
        // (...)
      }
     // (...)
}

@xavierleune
Copy link
Contributor Author

⏺ Here is the full story

Comparison:

  • Before (commit bcff473): if ($this->identityMap === true && is_int($table) === false)
  • After (commit 8a48050): if (is_int($table) === false)
Date Commit Action
Sept 2019 162f539 Bug introduced - references always stored
Aug 2020 bcff473 Fix - added if ($this->identityMap === true) before storage
Aug 2020 8a48050 Revert that reintroduced the bug - condition removed
Dec 2025 dc4de29 Final fix - added && $this->identityMap

What was happening

Even with identityMap disabled:

  1. Storage: Entities were always stored in $this->references
  2. Retrieval: During rehydration, the code checked $this->identityMap and cloned the object if disabled

So the behavior appeared correct on the surface (objects were cloned), but memory was still being consumed by storing all references. This fix now prevents storage when identityMap is disabled.

@xavierleune xavierleune requested a review from vgreb December 16, 2025 16:25
Copy link
Contributor

@vgreb vgreb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it ! Thank you for this fix.

@vgreb vgreb merged commit 034274e into ccmbenchmark:master Dec 16, 2025
10 checks passed
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.

2 participants