Perf improvement #446

merged 1 commit into from Sep 9, 2012


None yet
4 participants

My unit tests still pass

This pull request passes (merged f9a0906 into 441f969).


willdurand commented Aug 14, 2012

Heya, thanks for this performance patch. Could you explain why it's faster than before? Do you have a benchmark for example? I really wonder if we should skip this condition.

I opened (and then closed, sorry) an issue related to this patch. I was doing some profiling (xdebug + wincachegrind) of an application I'm working on that's built on top of Propel. I saw a significant chunk of time being used up by calls to XXXTableMap::initialize()

It seems that Propel is set up to try to avoid constructing a given TableMap subclass multiple times, but I did some debugging and was DEFINITELY seeing the same TableMap class initialized several times if an app used it more than once. In my schema.xml, all the tables have a schema attribute, like so:

<table name="tablename" schema="schemaname">

In my case the $name getting passed to DirectoryMap::hasTable() is always formatted as "schemaname.tablename" and the code I commented out was truncating that string to "schemaname" and attempting to look that up in its map of already-initialized maps. This caused the hasTable() check to always fail, and new map objects were being initialized at every opportunity.


willdurand commented Aug 20, 2012

It makes sense.. I will try your PR before to merge it.


fzaninotto commented Aug 22, 2012

any news on this PR?

jgadling commented Sep 8, 2012

Bump.... The patched version seems to be working well for me.

@willdurand willdurand added a commit that referenced this pull request Sep 9, 2012

@willdurand willdurand Merge pull request #446 from treylane/master
Perf improvement

@willdurand willdurand merged commit f580869 into propelorm:master Sep 9, 2012

1 check passed

default The Travis build passed

willdurand commented Sep 9, 2012

Merged, thanks!

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