[$200] Post performance updates

[jmgasper]

@atelomycterus - As discussed with the performance, there may be a couple things we can do to help with post delays. Can you implement something that may help here please?

[jmgasper]

Challenge https://www.topcoder.com/challenges/ced12554-3a2e-4f8b-9a78-128ba5f0cb43 has been created for this ticket.

This is an automated message for ghostar via Topcoder X

[jmgasper]

Challenge https://www.topcoder.com/challenges/ced12554-3a2e-4f8b-9a78-128ba5f0cb43 has been assigned to obog.

This is an automated message for ghostar via Topcoder X

[atelomycterus]

@jmgasper
How reproduce it:

• 20-30+ users are watching a category, post a new comment in the category

https://app.tideways.io/o/Topcoder/Discussions/trace/qsSby3gBhnQ2FlZ7mdQN/

#1 PDOStatement::execute
#2 Gdn_Database::query
#3 Gdn_SQLDriver::query
#4 Gdn_SQLDriver::insert
#5 ActivityModel::save
#6 ActivityModel::saveQueue
#7 CommentModel::notifyNewComment
#8 CommentModel::save2
#9 PostController::comment2
#10 Gdn_Dispatcher::dispatchController

[atelomycterus]

@jmgasper I am working on it. Notifications can be sent with a cron job in Vanilla. I'll update with details tomorrow.

[jmgasper]

Perfect, thanks!

[atelomycterus]

@jmgasper Please apply PR-#574. Thanks! I’ve enabled Feature.deferredNotifications.Enabled in Vanilla config. Please read my report on the current system performance when sending emails.

The main problems with email notifications

Email notifications are currently queued up and sent in the middle of the request, after the post has been saved and before the post is rendered. Vanilla has Feature.deferredNotifications.Enabled flag which moves executing of ActivityModel queue to the local scheduler. The intent is to remove sending notifications as a blocking operation for responses. If we enable this flag, then we put this problem aside a little.

The main problems with email notifications in Vanilla:

• Email notifications are currently inefficiently sending one-at-a-time.
• Batch-sending of notification emails is not implemented in Vanilla 3.3.

We haven’t seen this issue because groups have only 5-13 members on DEV. So the response times are <= 1 secs.

Sending emails (PROD)

This section provides a small report on the current system performance when sending emails. Therefore, it is necessary to fix this problem before the implementation of the notification tasks (#522 (comment))

On PROD
1 challenge group with 252 members
6 challenge groups with 100-160 members.

In other words, X users are watching a category, Vanilla will notify at least X users when a new comment/discussion posted. There may be more users in some cases.

The response times:
40-60 members in a group - 5-8 secs
120+ members in a group - 17 secs - 20 secs
150+ members in a group - 19 - 23 secs.
The more users in a group, the more actively they post comments.
Sending an email takes 0,1-0,13 secs for one member. Sending email notifications for 120 members takes about 14.4 secs.

You can see the 'WAIT' labels in Tidewyas. This label is displayed when the trace has significant unaccounted CPU wait time. You can also see a big time gap for the posting requests:

Posting a discussion/comment in a group with 100+ members:

I’ve posted a comment in a group with 120+ members to create a callgraph on PROD. 120 members were notified. The response time was 16.8 secs. Sending emails was 14.4 secs: https://app.tideways.io/o/Topcoder/Discussions/trace/HL2e0HgBmm2oVpuHKduw

The traces for other requests
https://discussions.topcoder.com/group/members/833 - 127 members
https://app.tideways.io/o/Topcoder/Discussions/trace/Aq0p0HgBmm2oVpuH9iZD

https://discussions.topcoder.com/group/764 - 157 members
https://app.tideways.io/o/Topcoder/Discussions/trace/yipLt3gBhnQ2FlZ7DDgh

Scheduler

Vanilla Scheduler adds a Message Queue functionality. The scheduler takes jobs during the normal request processing, and dispatches them at the end of the request.

Using Feature.deferredNotifications.Enabled=true will create a job to send email notifications. Again, if we enable this flag, then we put this problem aside a little. Sending emails will only run in a different thread, but it is also be slow.

The callgraph of a job if Feature.deferredNotifications.Enabled=true (my local env):

Solution

Sending notifications in fewer emails will improve performance. Users having the same notification regarding the same record will be lumped into email messages as BCC recipients in batches. Users are grouped on emails based on the notifications sharing the following: activity type, record type, record ID, subject and body. If all these align, emails are reused for up to X users.

This includes a refactor of emailing in ActivityModel. No functional changes should be observed here for the current notification email flow. These changes should only impact batch email sending. Since this feature only affects comments and discussions

Testing

It'd be better to create several groups with 50-200-500-1000 members for testing on DEV. If testers need help to generate data let me know. I’ll write a sql script to generate ‘dummy’ data. From a functional point of view, there is no difference, but we can see performance problems in advance.

[jmgasper]

@atelomycterus - Thanks for this, it makes sense. From a user standpoint, if we enable Feature.deferredNotifications.Enabled=true, will there be an immediate benefit when posting? I don't really care if the emails are sent a minute or two later than they are now - in practice that won't be an issue at all.

[atelomycterus]

@jmgasper

From a user standpoint, if we enable Feature.deferredNotifications.Enabled=true, will there be an immediate benefit when posting?

Yes, the response times will be shorter for groups with more participants.

I don't really care if the emails are sent a minute or two later than they are now - in practice that won't be an issue at all.

Sending emails will happen almost immediately, but in a new thread.
The delay feature was not implemented in Vanilla 3.3. The Scheduler class has the delay parameter in the addJob method signature, but it turned out that this functionality was not implemented. I've checked Vanilla github, the Scheduler was added on the eve of the 3.3 release.

Let me know if I should work on batch-sending of notification emails or we will put it aside for now and do tasks with March 2021 Shapeup label.

[jmgasper]

@atelomycterus - Let's just enable Feature.deferredNotifications.Enabled=true and get that deployed to dev for testing, and then focus on the rest of the March 2021 Shapeup, thanks.

We'll test in prod (another deployment on Sunday) and see if we need to do the batch-sending, thanks!