Ticket #280 (closed task: fixed)

Opened 4 years ago

Last modified 18 months ago

Add identity map to ensure unique object instances

Reported by: hans Owned by: hans
Priority: normal Milestone: 1.3
Component: Generator Version: devel
Severity: normal Keywords:
Cc: oliver@…

Description (last modified by hans) (diff)

See Oliver's good discussion of this issue at wiki:Development/UniqueInstances

Also see #187 for an example implementation of the needed getPrimaryKeyHash() method.

Although Oliver's patches were for the Propel 1.2 branch, the basic internals of Propel object hydration are basically the same in 2.0 -- and hence the implementation he suggests on the wiki is almost completely what we need.

Here is a distilled list of requirements for this task:

  • Add getPrimaryKeyHash($row, $startcol) to the Peer classes.
  • Add an $instances array to the Peer classes.
    • Note: I think it makes sense to keep this in the Peer classes because the Peer classes provide a natural way to categorize the classes. In particular, there could be issues with correctly categorizing instances of inherited classes. We do have to make these $instances maps public, however.
  • Add logic to the retrieveByPK(), populateObjects(), and doSelectJoin*() methods to first check whether the instance was loaded before hydrating the object.
    • Note: I don't think it's worth bothering with a new version of retrieveByPKs() because that method just calls doSelect() which is going to pull objects from the identity map if they already exist.
  • Finally, we should be able to simplify much of the kludgy code in Propel 1.2 that exists to prevent duplicate object instances in object trees, etc.

Also, for the implementation of getPrimaryKeyHash(), I think it makes sense to use serialize() instead of implode() for the multi-column primary key. Using serialize() is marginally slower, but it prevents problems in the case where a primary key might actually have '.' characters in it (this is not completely out of the question, as, for example, the node classes use '.' to delimit tree paths).

Change History

comment:1 Changed 4 years ago by hans

  • Type changed from defect to task
  • Description modified (diff)

comment:2 Changed 4 years ago by hans

  • Cc oliver@… added
  • Status changed from new to assigned

comment:3 Changed 4 years ago by hans

Added a first-cut implementation in [409]

comment:4 Changed 4 years ago by hans

Removed the change to retrieveByPK() to address ticket:290 in changeset:424. Basically, we must rely on the database for retrieveByPK() because the row may have been deleted, but is still being stored locally.

comment:5 Changed 4 years ago by hans

We need to consider when items should be removed from the instance map. We probably should at least provide a method to flush the cache. This is especially relevant for those who are using Propel in non-web applications.

comment:6 Changed 4 years ago by oliver

is this 2.0 or possible for 1.3 now that 1.3 contains some other substantial changes like PDO?

Myself and Tomek would be keen to contribute to getting this integrated into 1.3 if that's OK?

comment:7 Changed 4 years ago by david

Good question. I'm about to try making some changes to the hydration process in order to increase performance. Could you wait until after I committed that? I'm afraid we'd create plenty of conflicts otherwise.

comment:8 Changed 4 years ago by hans

I will wait until your changes are committed / stable. The actual changes to the builders were pretty trivial to enable the identity map.

comment:9 Changed 4 years ago by oliver

Ok, that's fine. Let us know when you're ready.

Slightly off topic:


While on hydration, we noticed that the new PDO based hydration does not cast the "always string" sql values into their appropriate php types like the creole implementation used to (eg rs->getInt()).

Was this an intentional omission (it will certainly make hydration faster), or are you considering something like  http://php.net/settype?

comment:10 Changed 4 years ago by david

I noticed the same while looking at the code and was just about to test that to verify it. This is a serious issue IMO. I'll write a mail to the list. Thanks for confirming this, I don't have to myself then ;)

comment:11 Changed 4 years ago by hans

Added preliminary implementation of this to the 1.3 branch in changeset:481.

This implementation includes some generated Peer methods to add or remove objects from the cache:

<?php

MyPeer::addInstanceToPool($obj);
MyPeer::removeInstanceFromPool($objOrPK);
MyPeer::clearInstancePool();

I also plan to add a reload() method to the object classes to force a refresh of a particular object from the database, but that hasn't been completed at this point (and shouldn't be necessary given the pool methods above).

comment:12 Changed 4 years ago by hans

In changeset:491 added object pool registration in the save() method. This now means:

<?php

$b = new Book();
// set values ...
$b->save();
$bid = $b->getId();

$b2 = BookPeer::retrieveByPK($bid);
// $b2 is same object as $b ($b2 === $b)

comment:13 Changed 3 years ago by hans

  • Status changed from assigned to closed
  • Resolution set to fixed
  • Milestone changed from 2.0 to 1.3

Ok, I think this is pretty finished now. Probably still a couple bugs, but I think with a combination of documentation and utility methods like reload() this won't cause people problems.

Note: See TracTickets for help on using tickets.