Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MD5 performance #3

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

MD5 performance #3

wants to merge 22 commits into from

Conversation

Opencontent
Copy link
Contributor

This pull-request boosts cluster performances (DB queries: durations) by having php (vs. postgres) calculating md5, together with some fixes (typos & phpdoc)

@francescor
Copy link

+1
this code has been already tested these days, but another series of test will be performed now. Any feed is really welcome.

@@ -1494,9 +1562,15 @@ public function _startCacheGeneration( $filePath, $generatingFilePath )
$query = 'INSERT INTO ' . $this->dbTable( $filePath ) . ' ( '. implode(', ', array_keys( $insertData ) ) . ' ) ' .
"VALUES(" . implode( ', ', $insertData ) . ")";

try {
//per testare scommenta la riga 1503 e sposta righe 1516-1548 fuori dal catch @todo
Copy link

Choose a reason for hiding this comment

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

mmmm :-)

@gggeek
Copy link

gggeek commented Jul 8, 2015

Quite a lot of changes for a single pull request

@francescor
Copy link

many thanks, Gaetano!
should we split this pull request in smaller ones?

@andrerom
Copy link
Contributor

andrerom commented Jul 8, 2015

@francescor yes, the PR says md5 being the topic, trying to solve everything and the kitchen sink in the same PR is not the way to go. Not easy to review and some of the fixes we might object to so then it delays the fix which is the important part. seems to be material for 4-5 PR's here; phpdoc, cs, md5, some return code changes, ...

@francescor
Copy link

ok, we'll do it..
in the meanwhile we've received good feeds from a pretty serious setup that have this code now: total runtime from 15s to 4s, so it's not peanuts ;)

@francescor
Copy link

(yes, we can confirm: cluster db queries duration from 33 secs to 1.5 secs)

@gggeek
Copy link

gggeek commented Jul 9, 2015

wow - seems like the postgresql query planner needs some serious help with optimizing away functions which have deterministic results... (either that or this is also an effect of the other changes you put in the PR).
A good tip for similar sql generated by the non-cluster php code, if any!

@francescor
Copy link

Gggeek, didn't get what you mean.. :(

@andrerom
Copy link
Contributor

He is referring to applying the same optimization other places (legacy kernel for instance), not applicable to this PR so you can safely move on splitting this up so we can merge it :)

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

Successfully merging this pull request may close these issues.

5 participants