Issue Details (XML | Word | Printable)

Key: MDL-14072
Type: Improvement Improvement
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: Eloy Lafuente (stronk7)
Reporter: Wen Hao Chuang
Votes: 0
Watchers: 4
Operations

Add/Edit UI Mockup to this issue
If you were logged in you would be able to see more operations.
Moodle

backuplib.php performance improvement for the function full_tag()

Created: 27/Mar/08 08:08 AM   Updated: 22/Apr/08 04:08 PM
Return to search
Component/s: Backup
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

Issue Links:
Relates
 

Participants: Eloy Lafuente (stronk7), Petr Skoda and Wen Hao Chuang
Security Level: None
QA Assignee: Petr Skoda
Resolved date: 19/Apr/08
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  « Hide
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!





 All   Comments   Change History   Version Control      Sort Order: Ascending order - Click to sort in descending order
Petr Skoda added a comment - 27/Mar/08 08:16 AM
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!


Eloy Lafuente (stronk7) added a comment - 27/Mar/08 08:46 AM
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?


Wen Hao Chuang added a comment - 03/Apr/08 05:37 AM
Added Martin and Anthony to the watch list just now. Eloy I think your suggestion is a great idea, other thought or comments? anyone?

Wen Hao Chuang added a comment - 18/Apr/08 02:35 PM
Also related to MDL-10770

Eloy Lafuente (stronk7) added a comment - 19/Apr/08 01:07 AM
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


Petr Skoda added a comment - 22/Apr/08 04:08 PM
reviewed, closing