postHydrate hook #463

Merged
merged 1 commit into from Oct 25, 2012

Projects

None yet

4 participants

@gertvr
Contributor
gertvr commented Sep 11, 2012

Added postHydate hook to make behaviors execute after loading an object.

This makes it possible to create a propel adapter for symfony bundles that currently only support doctrine (using doctrine's postLoad event).

(eg VichUploaderBundle)

@havvg havvg commented on the diff Sep 11, 2012
runtime/lib/om/BaseObject.php
@@ -208,6 +208,14 @@ public function postDelete(PropelPDO $con = null)
}
/**
+ * Code to be run after deleting the object in database
@havvg
havvg Sep 11, 2012 Member

This comment is not true, the phpDoc is wrong, too.

@havvg
Member
havvg commented Sep 11, 2012

Nice hook, this would help me reduce overwritten code of ENUM columns with MySQL a lot! +1

@gertvr
Contributor
gertvr commented Sep 11, 2012

I made a mistake creating a PR from a commit.
Not sure how to add a commit fixing the docs?
New PR?

@havvg
Member
havvg commented Sep 11, 2012

You can just commit and push to the same branch of this PR. The PR will update.

@gertvr
Contributor
gertvr commented Sep 11, 2012

I somehow managed to create a PR from a single commit. It's impossible to add new commits to the PR.

This is the commit that fixes the doc:
Tactics@1f86dc1

@fzaninotto
Member

Possible impact on performance. Can that be tested?

@gertvr
Contributor
gertvr commented Sep 12, 2012

I did the tests, there seems to be an expected, small impact.

I ran php-orm-benchmark with the following results:

                               | Insert | findPk | complex| hydrate|  with  |
                               |--------|--------|--------|--------|--------|
Propel 1.6                     |   3206 |   1681 |    442 |   1198 |   1316 |
Propel 1.6 with postHydrate()  |   3191 |   1673 |    421 |   1241 |   1311 |

These numbers are pretty consistent over several runs.
I'm pretty sure a 3% performance loss is not desirable. But is it a show stopper?

@fzaninotto
Member

Is there no other way to achieve what you want (as I understand,
compatibility with a Symfony2 Component), for instance by overriding the
hydrate() method manually in the stub class?

François

2012/9/12 Gert Vrebos notifications@github.com

I did the tests, there seems to be an expected, small impact.

I ran php-orm-benchmark with the following results:

                           | Insert | findPk | complex| hydrate|  with  |
                           |--------|--------|--------|--------|--------|

Propel 1.6 | 3206 | 1681 | 442 | 1198 | 1316 |
Propel 1.6 with postHydrate() | 3191 | 1673 | 421 | 1241 | 1311 |

These numbers are pretty consistent over several runs.
I'm pretty sure a 3% performance loss is not desirable. But is it a show
stopper?


Reply to this email directly or view it on GitHubhttps://github.com/propelorm/Propel/pull/463#issuecomment-8511020.

@willdurand
Member

We loose 3% in term of performance, just by adding a method call to a hook? oO

@gertvr
Contributor
gertvr commented Sep 13, 2012

It's a 3% performance loss when running the runHydrate test from the php-orm-benchmark. This hydrates the Book test class which has only 5 database fields. Obviously the percentage loss is significantly smaller for larger objects.

Still, there is an extra function call to the BaseObject::postHydrate stub method, a small impact is expected:

public function hydrate($row, $startcol = 0, $rehydrate = false)
{
    try {

        $this->id = ($row[$startcol + 0] !== null) ? (int) $row[$startcol + 0] : null;
        $this->title = ($row[$startcol + 1] !== null) ? (string) $row[$startcol + 1] : null;
        $this->isbn = ($row[$startcol + 2] !== null) ? (string) $row[$startcol + 2] : null;
        $this->price = ($row[$startcol + 3] !== null) ? (double) $row[$startcol + 3] : null;
        $this->author_id = ($row[$startcol + 4] !== null) ? (int) $row[$startcol + 4] : null;
        $this->resetModified();

        $this->setNew(false);

        if ($rehydrate) {
            $this->ensureConsistency();
        }
        $this->postHydrate($row, $startcol, $rehydrate);

        return $startcol + 5; // 5 = BookPeer::NUM_HYDRATE_COLUMNS.

    } catch (Exception $e) {
        throw new PropelException("Error populating Book object", $e);
    }
}

Should I run some other tests?

@fzaninotto: There're certainly other ways to achieve what I want by adding changes to stub classes, but then there is no way to build a behavior.

(It's comparable to Doctrine's onLoad event)

@fzaninotto
Member

I don't think more tests are necessary, but a decision must be made.

Note that if we push 10 changes with an impact of 3% each, the performance
loses about 35% overall.

Also, isn't there an alternative using the filter hook to build a behavior
for your purpose?

2012/9/13 Gert Vrebos notifications@github.com

It's a 3% performance loss when running the runHydrate test from the
php-orm-benchmark. This hydrates the Book test class which has only 5
database fields. Obviously the percentage loss is significantly smaller for
larger objects.

Still, there is an extra function call to the BaseObject::postHydrate stub
method, a small impact is expected:

public function hydrate($row, $startcol = 0, $rehydrate = false)
{
try {

    $this->id = ($row[$startcol + 0] !== null) ? (int) $row[$startcol + 0] : null;
    $this->title = ($row[$startcol + 1] !== null) ? (string) $row[$startcol + 1] : null;
    $this->isbn = ($row[$startcol + 2] !== null) ? (string) $row[$startcol + 2] : null;
    $this->price = ($row[$startcol + 3] !== null) ? (double) $row[$startcol + 3] : null;
    $this->author_id = ($row[$startcol + 4] !== null) ? (int) $row[$startcol + 4] : null;
    $this->resetModified();

    $this->setNew(false);

    if ($rehydrate) {
        $this->ensureConsistency();
    }
    $this->postHydrate($row, $startcol, $rehydrate);

    return $startcol + 5; // 5 = BookPeer::NUM_HYDRATE_COLUMNS.

} catch (Exception $e) {
    throw new PropelException("Error populating Book object", $e);
}

}

Should I run some other tests?

@fzaninotto https://github.com/fzaninotto: There're certainly other
ways to achieve what I want by adding changes to stub classes, but then
there is no way to build a behavior.

(It's comparable to Doctrine's onLoad event)


Reply to this email directly or view it on GitHubhttps://github.com/propelorm/Propel/pull/463#issuecomment-8520486.

@havvg
Member
havvg commented Sep 13, 2012

I can't believe that impact, what is the hardware that generates 3% performance decrease when only calling an empty method several times?

@gertvr
Contributor
gertvr commented Sep 13, 2012

@havvg You are right; I ran the tests again and again and again. I now cannot find any statistically relevant difference in performance between the two versions. Looks like the impact is near to nothing instead of 3 percent.

@fzaninotto I can't think of another way to have a behavior act whenever an object has been loaded/hydrated?

@fzaninotto
Member

@gertvr Using the filter hooks, you can do anything you like with some RegExp magic (see https://github.com/propelorm/Propel/blob/master/generator/lib/behavior/AlternativeCodingStandardsBehavior.php).

If there is no impact on performance, I'm OK with the change - but I'd like a confirmation.

@willdurand
Member

@gertvr could you confirm there is no impact on performance?

@gertvr
Contributor
gertvr commented Oct 25, 2012

@willdurand Tested this thoroughly: there is no impact.

@willdurand
Member

Great, so good to go ;)

@willdurand willdurand merged commit 65c62d4 into propelorm:master Oct 25, 2012

1 check passed

default The Travis build passed
Details
@willdurand
Member

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment