Code cleanup on greeting processing #24129
Conversation
|
(Standard links)
|
|
@johntwyman @andrew-cormick-dockery @anthonyblond you folks might be interested in this |
|
@seamuslee001 the test fails are hard to know what to do with - I removed an extraneous db lookup that is no longer required for the internal workings of the |
Test Result (5 failures / +5)api\v4\Action\UpdateContactTest.testUpdateWithIdInWhereapi\v4\Action\UpdateContactTest.testUpdateWithIdInValuesapi_v3_ContactTest.testCreateApiKey with data set "APIv3"api_v3_ContactTest.testCreateApiKey with data set "APIv4"CRM_Core_BAO_ActionScheduleTest.testContactModifiedAnniversaryTest Result (5 failures / +5) |
db1fd93 to
d2c423f
Compare
|
Note that pre-existing test cover is solid here. My performance testing has some queries eliminated here (perhtaps fifteen - this query output shows the queries before patch three, which eliminated the beige one) However, the performance problem is not notably improved & I'm going to do some php profiling since we are seeing a contact create take about three times as long (150 ms vs 45ms) and the queries only seem to add up to a couple of ms. My guess is that there might be something in there that is php-processing instensive and does not have caching. That is non-blocking on this. I am happy this is an improvement |
d2c423f to
1d0cfe9
Compare
|
This latest is giving me a significant improvement. My import test set add 500 contributions & updates 500 contacts. My best speed is 667 per minute - with The speed difference appears to be in the php layer, not the extra queries - with apiv4 metadata functions being a possible culprit - there is something weird in that 5 api calls on the same contact seems not much different to 1, - but one being markedly different to none. That would not be so odd except that it is on a per contact basis - perhaps some cache-clear per contact is an issue, but note that I have opportunistic cache flushing off... |
|
ohh fun - I just got it back to 667 per minute... with a cache fix... pushing up now |
|
oh wow - the cache fix with NO OTHER CHANGES gets me from 234 per minute to 577 per minute.... |
|
Fixed another cache miss - 612 per minute Note that I'm using Redis caching - for sites using Array caching there is likely to be no change in these caching fixes as the difference is use of the array cache facade |
|
test this please |
f0ae5fa to
171c6fb
Compare
171c6fb to
91ecf95
Compare
|
I've moved all the other commits out of this PR & this PR is now primarily a refactor / code cleanup of legacy functions. It WILL remove some db queries but these proved to be the least compelling of the performance improvements I trialled. Note test cover is super solid on this - including in the tests 'touched' in this PR (I only removed incorrect throws tags) |
| $row = $tokenProcessor->getRow(0); | ||
| foreach ($greetings as $greetingKey => $greetingString) { | ||
| $parsedGreeting = CRM_Core_DAO::escapeString(CRM_Utils_String::stripSpaces($row->render($greetingKey))); | ||
| $updateQueryString[] = " $greetingKey = '$parsedGreeting'"; |
There was a problem hiding this comment.
@eileenmcnaughton the one thing I wonder bout here is should we do a test for checking if $parsedGreeting != current passed greeting?
There was a problem hiding this comment.
hmm yeah - could do - I think it is no change at the moment - ie the check is not there already - so I'd probably look at it as a follow up
|
ok test coverage should be fine here |
Overview
Code cleanup on greeting procssing
Before
Each of the three greetings parsed separately, with separate calls to the greeting processor
After
one combined call
Technical Details
Ideally
sort_name&display_namewould be added in - but let's see what happens...Comments
I thought this would speed up performance but in testing so far it doesn't - however, the processing of greetings has a significant speed impact so I'm investigating why - which doesn't negate this PR as a code cleanup / move away from legacy functions