CPU % runs high even on an idle server caused by Custom Cert email task

[DeRespinis]

Moodle 4.1 and Moodle 4.0
Custom Cert Version 4.02 2022041902

CPU % runs high even on an idle server caused by Custom Cert email task.

The CPU % ranges from 0% to 3% on an idle machine with the Custom Cert email task disabled.
Mostly sits at 0%

The CPU % ranges from 8% - 15% on an idle machine with the Custom Cert email task enabled.
The CPU never stops and operates at a minimum of 8%

I noticed the change when I updated from 3.7 to 4.0. I went from 3.7 to 3.9 to 3.11 then to 4.0 and 4.1.

The Email task does not report any errors but the time to complete each task is taking about 20 seconds.

I have this running on both an 4-5 year old AWS Linux 2 LAMP and a new AWS Bitnami Moodle site.

This does not seem like correct behavior.

Thank for your help.

[mdjnelson]

It's a pretty complex task that tries to achieve a lot. I would be happy to merge any code that improves this task (maybe adding some caching?).

[Peterburnett]

Hi Folks,

I've stumbled here after an upgrade to 4.1 from 3.9, where I am seeing the email task use 4 million DB queries every run, while not actually issuing any certs at all.

After digging around on the Moodle tracker, it appears that 3.10 changed get_dynamic_info and introduced a fairly heavy overhead.

https://tracker.moodle.org/browse/MDL-72402

After doing a perf dump using tool_excimer, the flamegraph is basically all just stuck calculating uservisible here:

https://github.com/mdjnelson/moodle-mod_customcert/blob/MOODLE_402_STABLE/classes/task/email_certificate_task.php#L127

This is compounded because it needs to run a heavy calculation for visibility for n users, across n certificates, leading to a fairly topheavy n^2 complexity. After digging more into the code it seems fairly obvious we should filter basically everything we can before we check visibility for user, as it is by far the most heavy operation per user.

Additionally, if we can filter the outer loop down, the weight of the task can be greatly reduced. By introducing a time constraint on the certificate activities in scope, the calculation complexity can be limited to just the certificates that may potentially be in use.

In testing I introduced a constraint where at least one user had to have accessed the containing course between the run time of the previous task, and the next one. The results speak for themselves.

Im pretty sure this logic is correct, so I will PR the changes I have made here, but @mdjnelson Some feedback on whether we can actually trust this course access constraint would be appreciated, as I'm not across all the edge cases in customcert.

[mdjnelson]

Thanks @Peterburnett! Will look into this soon. Hoping to get a new release out with some big issues people are wanting resolved.

[mohamedmohamedatia]

I propose considering the following workarounds if you think they are useful or may help optimize this task:

1- Batch Processing:
-Modify the task to handle certificates in batches during each run.
-Implement a mechanism to resume processing from the last stopped point in subsequent runs.

2- Admin Configurable Options:
-Allow administrators to exclude certain older courses from certificate checks, especially those that have been closed for an extended period.
-Introduce parameters allowing administrators to specify the number of certificates processed per run, with a default processing rate (e.g., 10 to 50 certificates) that can be handled in a minute before the next run.

3- Two-Task Approach:
-Divide the task into two components:
a. One task for processing a limited number of certificates at regular intervals.
b. Another task for periodic, comprehensive checks of all certificates, perhaps on a weekly basis.

These suggestions aim to enhance the efficiency of the email certificate task, providing administrators with more control over the processing load based on their environment and requirements.

[mohamedmohamedatia]

Hello Everyone,

I've implemented some of the suggestions mentioned in my previous reply here.

While conducting a thorough code review, I identified an opportunity to reduce the number of database calls. In the existing code, there was a loop over all enrolled users in the course for each certificate. However, I modified it to iterate only over users who have access to the certificate. This adjustment significantly reduces the number of database read and write queries, particularly in scenarios where multiple certificates exist per course (e.g., one for students and one as a thank-you certificate for teachers). In such cases, the loop to email thank-you certificates will now only iterate over teachers instead of all users. Additionally, if we have 1000 students enrolled and only 200 users have access to the certificate, the loop will now run over 200 users only.

Additionally, I optimized the retrieval of users with the "manage" capability by fetching them once per certificate and storing them in an array. This approach eliminates the need to fetch users with this capability for each iteration in the user loop.

Although I didn't make changes to the code related to the comment "// Ensure the cert hasn't already been issued, e.g., via the UI (view.php) - a race condition," I believe removing this redundant check could further enhance performance. This check currently contributes to approximately 10% of database reads. However, I would appreciate input on whether this rare condition occurs and if users would receive two different copies of the certificate or two emails with one copy.

You can access the improved code in this GitHub fork: moodle-mod_customcert.

I've attached a PDF detailing the improvements in terms of database read/writes and processing time reductions.
Custom Certificate performance comparison - 600 Certificate.pdf

[mdjnelson]

Thanks @mohamedmohamedatia! I have created #610 on your behalf. I will leave some comments here for you to have a look at.

For future reference if you are making changes you should create a different branch.

[mohamedmohamedatia]

Thanks, Mike, for creating issue #610 and for your feedback. I'm new to contributing to Moodle plugins, and this was my first attempt. I'll follow your suggestion to create a separate branch for future changes.

[mdjnelson]

You're welcome. Also, just fyi it's Mark - but Mike is close enough. 😄

[mohamedmohamedatia]

My apologies for the mix-up! I'll make sure to remember it's Mark from now on. 😄

[mdjnelson]

There are now three different pull requests for the issue #531.

1. Improvement to the speed of the emailing certificate task (#531) #610
2. 531: Optimizing email and issuing task.  #632
3. Optimize task email_certificate_task #634

This will take time for me to look at 3 of them. Is it possible that all 3 of them will work together or do I need to decide? I do not have that much time on this project as I used to.

[mohamedmohamedatia]

Hi Mark and Team,

I apologize for the delayed response. I was on an extended leave for two months and was unable to review emails during that time.

We are nearly finished with #610, as I have incorporated your feedback and clarified some points regarding course visibility and end dates. I suggest we proceed with #610 for now and address #632 as future enhancements, as there is still potential for improvements that could be covered by #632.

I will review #632 and #634 Later. and get back to you.

This was my first experience participating in a Moodle plugin, and I have had a valuable learning experience from working on this project and receiving Mark’s feedback. I am hopeful that the changes I’ve made will improve the plugin’s performance.

Please let me know if you have any comments or alternative scenarios to consider.

[mdjnelson]

Hope you enjoyed your holiday @mohamedmohamedatia.

[mohamedmohamedatia]

Yes @mdjnelson, I really enjoyed traveling back to my home country and seeing my family.

[dvdcastro]

Ahoy @mdjnelson and @mohamedmohamedatia !

Hey guys, Should we close #632?

Do you guys need anything from us to move #610 forward?

[mdjnelson]

Hi @dvdcastro, not just yet. I am going to first review and test #610 then look at the changes you have done on top of @mohamedmohamedatia and review them separately. Thanks.

[mdjnelson]

Released in the plugins DB, closing. Thanks all.