Moodle

backuplib.php performance improvement for the function full_tag()

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.5, 1.5.1, 1.5.2, 1.5.3, 1.5.4, 1.6.6, 1.7.4, 1.8.5, 1.9
  • Fix Version/s: 1.8.6, 1.9.1
  • Component/s: Backup
  • Labels:
    None
  • Affected Branches:
    MOODLE_15_STABLE, MOODLE_16_STABLE, MOODLE_17_STABLE, MOODLE_18_STABLE, MOODLE_19_STABLE
  • Fixed Branches:
    MOODLE_18_STABLE, MOODLE_19_STABLE

Description

OK I noticed that this got improved in 1.9, but not 1.5.x - 1.8.x. Maybe we should consider backport this back to 1.5 - 1.8 (I could help with this).

Basically the problem is that if you have really BIG course (e.g. for our case, a course with 1300+ students with lots of quizzes and unlimited quiz attempts for practice quizzes..etc.). In this case for moodle 1.5. - 1.8 the backup process would stop when the system is "Writing categories and questions." We tried to narrow down the problem and one thing that we came up with is to improve the function full_tag a little bit.

Currently in the backuplib.php in moodle 1.5.x to 1.8.x, it looks like this:

function full_tag($tag,$level=0,$endline=true,$content,$attributes=null) { global $CFG; //Here we encode absolute links $content = backup_encode_absolute_links($content); $st = start_tag($tag,$level,$endline,$attributes); $co = xml_tag_safe_content($content); $et = end_tag($tag,0,true); return $st.$co.$et; }

This is not efficient as no matter what the backup_encode_absolute_links will be called. We rewrote it a little bit to add a if condition before the $content = backup_encode_absolute_links($content); See below:

439 //Here we encode absolute links
440 //Only run backup_encode_absolute_links when necessary...
441 //Running backup_encode_absolute_links on every tag adds a lot of overhead
442 //Looking at backup_encode_absolute_links, it appears to only operate on
443 if (strpos($content, "file.php") !== false || strpos($content, "index.php") !== false || strpos($content, "discuss.php"))
444 { 445 $content = backup_encode_absolute_links($content); 446 }

The solution that moodle 1.9 came up with is: (see MDL-10770 for more details):

function full_tag($tag,$level=0,$endline=true,$content,$attributes=null) {

global $CFG;
//Here we encode absolute links
// MDL-10770
if (is_null($content)) { $content = '$@NULL@$'; } else { $content = backup_encode_absolute_links($content); }
$st = start_tag($tag,$level,$endline,$attributes);

$co = xml_tag_safe_content($content);

$et = end_tag($tag,0,true);

return $st.$co.$et;
}

Which one would be a better solution? Just thought that I would throw it out here for discussion. I could help with backporting by the way once we reach some kind of agreement. thanks!

Issue Links

Activity

Hide
Petr Škoda (skodak) added a comment -

if (strpos($content, "file.php") !== false || strpos($content, "index.php") !== false || strpos($content, "discuss.php"))
this would not IHMO work, because modules are free to relink links to any *.php files
I think there should be an option to disable the relinking by using extra param - not all fields need that

my -1 for backporting,
we should imo try to improve this substantially in 1.9.x or 2.0

thanks for the report!

Show
Petr Škoda (skodak) added a comment - if (strpos($content, "file.php") !== false || strpos($content, "index.php") !== false || strpos($content, "discuss.php")) this would not IHMO work, because modules are free to relink links to any *.php files I think there should be an option to disable the relinking by using extra param - not all fields need that my -1 for backporting, we should imo try to improve this substantially in 1.9.x or 2.0 thanks for the report!
Hide
Eloy Lafuente (stronk7) added a comment -

Yup, 100% agree with Petr, interlinking is really complexer that that. The list of possible destinations is far bigger that those 3.

Anyway, after a quick thought... I think we could save millions of backup_encode_absolute_links() calls by simply checking for empties and numbers. Sounds really simple and safe to implement.

Comments?

Show
Eloy Lafuente (stronk7) added a comment - Yup, 100% agree with Petr, interlinking is really complexer that that. The list of possible destinations is far bigger that those 3. Anyway, after a quick thought... I think we could save millions of backup_encode_absolute_links() calls by simply checking for empties and numbers. Sounds really simple and safe to implement. Comments?
Hide
Wen Hao Chuang added a comment -

Added Martin and Anthony to the watch list just now. Eloy I think your suggestion is a great idea, other thought or comments? anyone?

Show
Wen Hao Chuang added a comment - Added Martin and Anthony to the watch list just now. Eloy I think your suggestion is a great idea, other thought or comments? anyone?
Hide
Wen Hao Chuang added a comment -

Also related to MDL-10770

Show
Wen Hao Chuang added a comment - Also related to MDL-10770
Hide
Eloy Lafuente (stronk7) added a comment -

Hi,

now both:

  • backup_encode_absolute_links() at backup
  • restore_decode_absolute_links() at restore

prevents further execution if the content is NULL, empty string or number. Hopefully this'll save a bunch of cycles (specially in backup).

Applied to 18_STABLE, 19_STABLE and HEAD. Ciao

Show
Eloy Lafuente (stronk7) added a comment - Hi, now both:
  • backup_encode_absolute_links() at backup
  • restore_decode_absolute_links() at restore
prevents further execution if the content is NULL, empty string or number. Hopefully this'll save a bunch of cycles (specially in backup). Applied to 18_STABLE, 19_STABLE and HEAD. Ciao
Hide
Petr Škoda (skodak) added a comment -

reviewed, closing

Show
Petr Škoda (skodak) added a comment - reviewed, closing

People

Dates

  • Created:
    Updated:
    Resolved: