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

Improve performance on getSoftContribution details - only run one query instead of one per contribution #14747

Merged

Conversation

@mattwire
Copy link
Contributor

commented Jul 7, 2019

Overview

When calling the CRM_Contribute_BAO_ContributionSoft::getSoftContribution() function for multiple contributions it gets pretty inefficient as a separate query is run every time. When retrieving bulk contributions we can improve performance significantly by just running the query once for all contribution IDs.

Before

Execution time (first run, second run):
0.053346, 0.043242

After

Execution time (first run, second run):
0.0052369999999999, 0.00575

About 10 times faster on my test system!

Technical Details

Create a new function that runs the query for multiple contributions instead of calling a function that runs the query separately for each contribution.

Comments

Currently I've only switched over api3 Contribution.get but there are multiple other places where this could be switched over and might improve performance. @eileenmcnaughton @pfigel This was something I noticed when doing some other work and I couldn't resist trying to fix... But I don't actually need it.. so if either of you have an interest in picking this up that would be great :-)

@civibot

This comment has been minimized.

Copy link

commented Jul 7, 2019

(Standard links)

@civibot civibot bot added the master label Jul 7, 2019

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

test fails relate - but it looks like just a case of an early return when empty.

@mattwire mattwire force-pushed the mattwire:contributionapi_getsoftperformance branch 2 times, most recently from 4d4990e to a3c42ca Jul 8, 2019

@mattwire

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

@eileenmcnaughton @colemanw There are 4 remaining test fails - they are real because this PR changes the return values from a 1-based array to a 0-based array and the tests are looking for array element 1.

There are two options:

  1. Modify the PR to return a 1-based array for the list of softcredits (it's just a sequential array of softcredits).
  2. "Fix" the tests to use CRM_Utils_Array::first and not care about the numeric keys on the array.

I think we've done 2. before as sequential should really be 0-based but I can't remember where.

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

@mattwire I'm kinda past the point of wanting to change anything at all in apiv3 - the sooner we completely freeze it the better IMHO. We just need @colemanw to do a few more fixes on apiv4 & we can ship it with core & freeze apiv3.

In thee meantime we should keep changes to a minimum- so I guess we can make this change but we shouldn't do any standardisation on the indexing

@mattwire mattwire force-pushed the mattwire:contributionapi_getsoftperformance branch from a3c42ca to 5062a7b Jul 28, 2019

@eileenmcnaughton eileenmcnaughton merged commit c278d15 into civicrm:master Jul 29, 2019

1 check passed

default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.