CRM-21260: CiviMail compose UI very slow to initialize, CRM-21316: Refactor getRecipients fn #11142

monishdeb · 2017-10-16T06:14:35Z

Overview

This PR is intended to reduce slowness of CiviMail screen during initialization and drafting, by addressing various issues:

Before

AngularJs use Mailing.create API to fetch recipients in an inefficient way, which are:
a) Create unnecessary mailing Job later used by getRecipients(...) to fetch recipients
b) Cause rollback on DB changes made after creating dummy mailing, it's jobs and mailing_recipients records. See here
c) getRecipients(...) is an expensive function as in underlying code it executes some queries needlessly and also dependent on mailing job (only used in temp table name) which is unnecessary.
Reduce AJAX calls while CiviMail initialization and drafting. Some of the REST API are triggered needlessly and causes an issue like:
REST API call for Mailing.getlist({"params":{"id":{"IN":[]}},"extra":["is_hidden"]})
Response: {"is_error":1,"error_message":"invalid criteria for IN"}

After

getMailRecipients(...) is created after simplifying the code of self::getRecipients(..) and these are the changes made:
a. Takes saved mailing object as the parameter later used mostly to build filters.
b. This function ONLY fetch and store mailing recipients, doesn't handle SMS mode
c. Use fewer JOINs in underlying queries.
d. Simplifies code to fetch contacts from selected mailing smart group(s).
e. Doesn't execute queries needlessly, e.g.
i) if SG (smart groups) are selected then ONLY queries responsible to fetch contacts from SG are executed
ii) If no 'Include' groups are selected then simply return as this also means no groups are selected to fetch recipients.
f. Refreshes contact cache of all smart groups, at once.
g. Kept both functions for now to reduce any unintended regressions
This function is later called via mailing.create API using a special parameter
{get_recipients: TRUE} and removed force_rollback and removed dummy mailing, job create
Reduce AJAX calls (In progress.. for now fixed the error mentioned in 2nd issue)

Technical Details

Marked with [WIP] as optimisation needed and reduce more AJAX calls to improve performance.

JoeMurray · 2017-10-16T14:58:34Z

CRM/Mailing/BAO/Mailing.php

-   * @param bool $storeRecipients
-   * @param bool $dedupeEmail
-   * @param null $mode
+   * This function is created after simplifying the code of self::getRecipients(..) and these are the changes made


I would only put permanent documentation in the PR, and put descriptions of changes / improvements that are relevant while reviewing into comments on the PR. That way, the repo only has comments that continue to be relevant. So, for example, the PR should contain:
This function provides the recipients of a CiviMail mailing.
It provides a simplified implementation of an earlier function, now removed, self::getRecipients(..).
NB: The Smart Group contact cache is refreshed immediately by this call for all included and excluded smart groups in the mailing.

In comments on the PR, put in 1 - 5b.

Now, a question: do we really want this code to override the smart cache timeout on groups and force their refresh? Wouldn't it be better to rely on the cache than to force it to be rebuilt? We could provide an additional button for those rare cases where someone is concerned about forcing a rebuild, and willing to accept the possibly long rebuild time. Recall that this can take 5 - 10 minutes for some large complex hierarchical smart groups.

Actually, we are not forcing to rebuilt group contact cache because CRM_Contact_BAO_GroupContactCache::load($groupDAO) respects the cache and refresh time, and on basis of which it will decide to rebuild contact cache for smart group identified by $groupDAO which is how it's been done in other places.

JoeMurray · 2017-10-16T15:01:50Z

CRM/Mailing/BAO/Mailing.php

-    $job = CRM_Mailing_BAO_MailingJob::getTableName();
-    $mg = CRM_Mailing_DAO_MailingGroup::getTableName();
-    $eq = CRM_Mailing_Event_DAO_Queue::getTableName();
+    // there is no need to proceed further as no mailing group is selected to include recipients,


Change 'as' to 'if', or put this comment inside the if
// there is no need to proceed further if no mailing group is selected to include recipients,

JoeMurray · 2017-10-16T15:06:28Z

CRM/Mailing/BAO/Mailing.php

+
+      $groupDAO = CRM_Core_DAO::executeQuery($sql);
+      while ($groupDAO->fetch()) {
+        if ($groupDAO->cache_date == NULL) {


It seems like you are not rebuilding the cache for all groups, just for ones where the cache is empty/expired? If so, please clarify the DocBlock for the function.

Yes if cache is empty as per earlier code in getRecipients(...), I haven't changed this logic. Will mention it in doc block

JoeMurray · 2017-10-16T15:09:31Z

CRM/Mailing/BAO/Mailing.php

+    }
+
+    // Create a temp table for contact exclusion.
+    $excludeTempTablename = "excluded_recipients_mailing_temp";


This will cause errors if two people are sending at the same time. Could you make unique name using a UUID or something?

ok will use "excluded_recipients_mailing_temp" . substr(sha1(rand()), 0, 4)

JoeMurray · 2017-10-16T15:10:04Z

CRM/Mailing/BAO/Mailing.php

+
+    // Create a temp table for contact exclusion.
+    $excludeTempTablename = "excluded_recipients_mailing_temp";
+    $includedTempTablename = "included_recipients_mailing_temp";


Make unique eg by using UUID

JoeMurray · 2017-10-16T15:15:19Z

CRM/Mailing/BAO/Mailing.php

+    );
+    if (!empty($recipientsGroup['Exclude'])) {
+      $sql = sprintf("
+        INSERT INTO %s (contact_id)


Could you estimate how much memory these two temp tables might take based on number of groups and size of db (eg for 600k records, with a group involving 10 groups and total of 1.8M members), then post into dev channel for feedback on if this is problem? There are currently around 20 places in codebase where temp tables using ENGINE=HEAP are created.

JoeMurray · 2017-10-16T15:21:50Z

CRM/Mailing/BAO/Mailing.php

+        INSERT INTO %s (contact_id)
+          SELECT DISTINCT gc.contact_id
+          FROM  civicrm_group_contact gc
+          WHERE gc.status = 'Added' AND gc.id IN (%s)", $excludeTempTablename, implode(',', $recipientsGroup['Exclude']));


Are the names of Groups sanitized?

FWIW, the query-builder can handle INSERT...SELECT... with validation/escaping/interpolation:

CRM_Utils_SQL_Select::from('civicrm_group_contact') ->select('DISTINCT gc.contact_id') ->where('gc.status = "Added" AND gc.id IN (#groups)') ->param('#groups', $recipientsGroup['Exclude']) ->insertInto($excludeTempTablename, array('contact_id')) ->execute();

JoeMurray · 2017-10-16T15:39:52Z

CRM/Mailing/BAO/Mailing.php

-    $storeRecipients = FALSE,
-    $dedupeEmail = FALSE,
-    $mode = NULL) {
+  public static function getMailRecipients($mailingObj) {


Could you put into function signature DocBlock an explanation of what $mailingObj is, eg the current mailing object (as opposed to a former excluded mailing object)?

JoeMurray · 2017-10-16T15:40:04Z

CRM/Mailing/BAO/Mailing.php

+        FROM  civicrm_mailing_event_queue meq
+         INNER JOIN  civicrm_mailing_job mj ON meq.job_id = mj.id
+         INNER JOIN  civicrm_mailing_group mg ON mj.mailing_id = mg.entity_id AND mg.entity_table = 'civicrm_mailing'
+        WHERE  mg.mailing_id = %d AND mg.group_type = 'Exclude'", $excludeTempTablename, $mailingID);


I'm probably confused, but isn't $mailingID the ID for the current mailing, so the comment on L211 doesn't make sense to me. If $mailingID is the current mailing, then it seems to imply that the Mailing Event Queue has been built, which could be used to retrieve the recipients.

Yes you are right, why refactoring, I overlooked this query and simply included it from here without any thought. I will remove this query as it is no longer necessary here because we are building recipients new without any job or event queue.

JoeMurray · 2017-10-16T15:46:06Z

CRM/Mailing/BAO/Mailing.php

-    $mailing_id = NULL,
-    $storeRecipients = FALSE,
-    $dedupeEmail = FALSE,
-    $mode = NULL) {


Is sending via SMS currently using this function? Wouldn't the performance improvements be equally useful for SMS recipients?

For now to keep it simple I have created this new fn, only to fetch mail recipients, and for civiSMS it will use the old getRecipients() fn. Also this new fn is only called via civiMail screen, so once I am done with the refactoring will figure out a way to extend support for phone recipients.

JoeMurray · 2017-10-16T15:50:32Z

CRM/Mailing/BAO/Mailing.php

+    // Get all the group contacts we want to include.
+    $mailingGroup->query(
+      "CREATE TEMPORARY TABLE $includedTempTablename
+            (email_id int, contact_id int primary key)


Better to follow the convention of always putting primary key first to avoid confusing people, as you rely on contact_id being primary below when using a REPLACE query:
(contact_id int primary key, email_id int)

oh ok .. will swap.

JoeMurray · 2017-10-16T16:03:34Z

CRM/Mailing/BAO/Mailing.php

+    $groupBy = $groupJoin = '';
+    $orderBy = "i.contact_id, i.email_id";
+    if ($mailingObj->dedupe_email) {
+      $orderBy = "MIN(i.contact_id), MIN(i.email_id)";


I think using DISTINCT in the SELECT clause, a synonym for DISTINCTROW, is more efficient than doing this order by / group by as a method of eliminating duplicates. You'll still need the group by, but you can remove the effort to sort/order.

old cruft code :( will change

monishdeb · 2017-10-18T17:29:39Z

Jenkins test this please

seamuslee001 · 2017-10-18T20:44:29Z

Jenkins test this please

davejenx · 2017-10-27T12:07:38Z

@monishdeb I have tested this patch on the site that I mentioned in CRM-21260. I used Chrome, using Inspector -> Network to check the time elapsed until the UI finishes loading, including the WYSIWYG: in my testing this is always the last thing to load, immediately after the recipient count changes from "(Estimating)" to "No recipients".

Previous testing for CRM-21260 showed that the number of message templates makes a big difference. So I tested with & without the patch with different numbers of message templates enabled (all vs just the reserved ones). 3 trials in each condition.

Results

Testing 4.7.26RC-201710090252

Message templates: 31:
41s average (38s, 40s, 44s)
Total transferred: 150KB XHR / 1.3MB All
XHR requests: 22, biggest: 106KB builder?an=angular-modules.json
Biggest API response: 18KB (Mailing.getfields)

Message templates: 1212:
117s average (115s, 110s, 127s)
Total transferred: 150KB XHR / 1.4MB All
XHR requests: 22, biggest: 106KB builder?an=angular-modules.json
Biggest API response: 18KB (Mailing.getfields)

Testing 4.7.26RC-201710090252 + PR 11142

Message templates: 31:
38s average (38s, 35s, 40s)
Total transferred: 144KB XHR / 1.3MB All
XHR requests: 10, biggest: 106KB builder?an=angular-modules.json
Biggest API response: 18KB (Mailing.getfields)

Message templates: 1212:
115s average (114s, 122s, 110s)
Total transferred: 144KB XHR / 1.4MB All
XHR requests: 10, biggest: 106KB builder?an=angular-modules.json
Biggest API response: 18KB (Mailing.getfields)

Results Summary

The patch reduced the average initialization time by 2-3 seconds and the number of AJAX requests by 12.
The number of enabled message templates makes a much bigger difference to the initialization time.
Even with the patch and with only 31 message templates enabled, the load time of 38s seems very slow.
The great majority of the load time, as described in CRM-21260, seems to be client-side: during most of the initialization time the server is idle, no AJAX requests are in progress and the browser is using 100% CPU. With the patch in place, the biggest delay occurs between the end of the Mailing.getfields request and the start of the 2nd (of 3) Mailing.create POST.

davejenx · 2017-11-17T16:00:08Z

@monishdeb See #11267 for @nbrettell's work on loading message templates on demand. Getting excellent improvements in load time here.

eileenmcnaughton · 2017-12-01T22:39:24Z

test this please

eileenmcnaughton · 2017-12-01T22:42:50Z

@davejenx @monishdeb @mattwire @seamuslee001 (cc @totten since could affect mosaico)

A lot of time seems to have gone into testing & discussing this & my take is that it is possibly mergeable on test - pass (which needs to be confirmed) & that it has possibly stalled due to lack of clarity about status

seamuslee001 · 2017-12-01T22:49:47Z

@monishdeb can you rebase this please

…mplify getRecipients fn

monishdeb · 2017-12-19T15:23:29Z

ping @totten @seamuslee001 @lcdservices @colemanw @eileenmcnaughton @mlutfy

mlutfy · 2017-12-19T16:15:47Z

Code seems good to me. Big patch, but a lot of cleanup. However, I did not test.

@mattwire @davejenx Can you have one last look & confirm it's working with master?

Is anyone using this patch in production?

lcdservices · 2017-12-19T17:28:15Z

@mlutfy we have implemented the patch for NYSS and are planning to roll to production this week.

JKingsnorth · 2018-01-16T09:08:50Z

@lcdservices Has this been running OK in production?

lcdservices · 2018-01-16T12:10:42Z

@JKingsnorth yes, we've been running in a high volume multi-site environment for a couple weeks without issue

JKingsnorth · 2018-01-16T14:11:44Z

Great, is there any further testing or review required to merge this?

mlutfy · 2018-01-18T15:08:13Z

Merging based on the comments/reviews/testing. Thanks everyone!

monishdeb · 2018-01-18T17:12:00Z

Thank you everyone for your feedback and effort in getting this PR merged :)

davejenx · 2018-01-18T17:16:47Z

@monishdeb Thanks for all your work!

eileenmcnaughton · 2018-01-18T20:43:48Z

well done all

JKingsnorth · 2018-01-19T15:14:59Z

CRM/Mailing/BAO/Mailing.php

+    if (count($includeSmartGroupIDs)) {
+      CRM_Utils_SQL_Select::from($contact)
+        ->select("$contact.id as contact_id, $email.id as email_id")
+        ->join($email, " INNER JOIN $email ON $email.contact_id = $contact.id ")


Hi @monishdeb I've just spotted something - I think these changes will break trying to include a smart group in an SMS type mailing? There is no if statement, so this will insert email IDs into the 'phone ID' column in the temporary table?

I haven't proved this definitively yet, working on setting up my localhost to test.

Thanks, @JKingsnorth for notifying this break. I am going to a create a separate PR for this fix.

JKingsnorth · 2018-01-19T15:43:36Z

Unfortunately I haven't been able to test this on dmaster, because every time I try to create a new SMS mailing I hit a DB error. I'm not sure whether that's related to this PR or not yet.

However, from reading that part of the code, I think that this PR will introduce a problem in smart groups being included in SMS mailings.

eileenmcnaughton · 2018-02-08T02:07:03Z

CRM/Mailing/BAO/Mailing.php

@@ -1788,7 +1609,7 @@ public static function create(&$params, $ids = array()) {

    // Create parent job if not yet created.
    // Condition on the existence of a scheduled date.
-    if (!empty($params['scheduled_date']) && $params['scheduled_date'] != 'null' && empty($params['_skip_evil_bao_auto_schedule_'])) {
+    if (!empty($params['scheduled_date']) && $params['scheduled_date'] != 'null') {


I have opened #11653 to e-instate this check - I assume is was removed on the assumption that it was not being used but it's actually accessible via the api. We should merge that to 4.7.31

In `CRM_Mailing_BAO_Mailing::create` and `Mailing.create` API, there is a (*ahem*) special behavior where setting the `scheduled_date` will immediately trigger scheduling. One shouldn't submit `scheduled_date` for a preview. This was not symptomatic before civicrm#11142/v4.7.31 because all preview operations were wrapped in a transaction and rolled back. But now previews are allowed to have side-effects, so we need some other means to prevent. This copies the workaround from `crmMailingMgr.save()` and applies it to `crmMailingMgr.preview()` (etal).

monishdeb added the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Oct 16, 2017

monishdeb changed the title ~~CRM-21260: CiviMail compose UI very slow to initialize, CRM-21316: Si…~~ CRM-21260: CiviMail compose UI very slow to initialize, CRM-21316: Simplify getRecipients fn Oct 16, 2017

monishdeb force-pushed the CRM-21260 branch from 4be7756 to 6cbeeb4 Compare October 16, 2017 06:15

civicrm-builder added the master label Oct 16, 2017

monishdeb force-pushed the CRM-21260 branch 4 times, most recently from ed4c6f8 to 0033ace Compare October 16, 2017 07:38

JoeMurray reviewed Oct 16, 2017

View reviewed changes

monishdeb changed the title ~~CRM-21260: CiviMail compose UI very slow to initialize, CRM-21316: Simplify getRecipients fn~~ CRM-21260: CiviMail compose UI very slow to initialize, CRM-21316: Refactor getRecipients fn Oct 18, 2017

monishdeb force-pushed the CRM-21260 branch 2 times, most recently from d1a9119 to 7375f51 Compare October 18, 2017 16:26

monishdeb mentioned this pull request Oct 24, 2017

CRM-20955 CRM-20966 Adding unit tests. #11184

Merged

monishdeb removed the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Oct 24, 2017

davejenx mentioned this pull request Oct 27, 2017

CRM-21279: Rebuild recipient list and calculate count on demand, store result in cache #11091

Merged

monishdeb force-pushed the CRM-21260 branch from 7375f51 to 99f5f04 Compare October 28, 2017 15:37

monishdeb force-pushed the CRM-21260 branch from 8593859 to e769771 Compare December 4, 2017 10:50

monishdeb force-pushed the CRM-21260 branch from e769771 to a0d0a36 Compare December 12, 2017 16:40

monishdeb added 2 commits December 19, 2017 18:57

CRM-21260: CiviMail compose UI very slow to initialize, CRM-21316: Si…

6dd717a

…mplify getRecipients fn

test failure fix

6ec61f1

monishdeb force-pushed the CRM-21260 branch from 6cf019a to 6ec61f1 Compare December 19, 2017 13:29

avoid recipient rebuild on preview,sendTest and simple save

d0d7fea

mlutfy merged commit ad853d8 into civicrm:master Jan 18, 2018

monishdeb deleted the CRM-21260 branch January 18, 2018 17:00

JKingsnorth reviewed Jan 19, 2018

View reviewed changes

JKingsnorth mentioned this pull request Feb 5, 2018

CRM-21316: Add missing code to handle smart group on sms mode #11558

Merged

eileenmcnaughton reviewed Feb 8, 2018

View reviewed changes

mlutfy added this to the 4.7.31 milestone Feb 9, 2018

totten mentioned this pull request Mar 30, 2018

(dev/mail/5) "New Mailing" - Previews should not schedule real blasts #11904

Merged

CRM-21260: CiviMail compose UI very slow to initialize, CRM-21316: Refactor getRecipients fn #11142

CRM-21260: CiviMail compose UI very slow to initialize, CRM-21316: Refactor getRecipients fn #11142

Conversation

monishdeb commented Oct 16, 2017 • edited Loading

Overview

Before

After

Technical Details

Choose a reason for hiding this comment

monishdeb Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monishdeb Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

JoeMurray Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

monishdeb Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeMurray Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeMurray Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monishdeb commented Oct 18, 2017

seamuslee001 commented Oct 18, 2017

davejenx commented Oct 27, 2017

Results

Testing 4.7.26RC-201710090252

Testing 4.7.26RC-201710090252 + PR 11142

Results Summary

davejenx commented Nov 17, 2017

eileenmcnaughton commented Dec 1, 2017

eileenmcnaughton commented Dec 1, 2017

seamuslee001 commented Dec 1, 2017

monishdeb commented Dec 19, 2017

mlutfy commented Dec 19, 2017

lcdservices commented Dec 19, 2017

JKingsnorth commented Jan 16, 2018

lcdservices commented Jan 16, 2018

JKingsnorth commented Jan 16, 2018

mlutfy commented Jan 18, 2018

monishdeb commented Jan 18, 2018

davejenx commented Jan 18, 2018

eileenmcnaughton commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JKingsnorth commented Jan 19, 2018

Choose a reason for hiding this comment

monishdeb commented Oct 16, 2017 •

edited

Loading

monishdeb Oct 16, 2017 •

edited

Loading

monishdeb Oct 16, 2017 •

edited

Loading

JoeMurray Oct 16, 2017 •

edited

Loading

monishdeb Oct 16, 2017 •

edited

Loading

JoeMurray Oct 16, 2017 •

edited

Loading

JoeMurray Oct 16, 2017 •

edited

Loading