Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.3, 2.5
    • Fix Version/s: 2.4.4
    • Component/s: Libraries
    • Labels:
      None
    • Rank:
      50112

      Description

      The html_writer::attribute function is called thousands of times on typical pages (1,271 times on the one I'm testing, to be exact).

      This function includes a check, with developer debug warning, for if some idiot passes an array for the attribute value.

      Since the check has been there since html_writer was invented (actually before), I think people should really have noticed the developer debug warning now, and this code should be removed.

      According to my profiling this check adds ~2ms to a page load on OU systems. Not a lot but a pretty big chunk just for a never-used dev debug warning.

      (Note: I'm totally in favour of dev debug checks usually, just not in functions that are called thousands of times on many pages across the system.)

        Activity

        Hide
        Sam Marshall added a comment -

        note: Eloy points out that it's possible to do is_array faster:

        "casting to array and === is like 8 times faster than doing is_array(), btw.
        if ((array)$arr === $arr"

        However, I don't think there is a value to this check any more - maybe it was needed once but after several versions worth of warnings, it should be good to go. We don't check every random function argument to see if it's an array.

        I checked this on core systems and found similar results - a small but measurable performance impact, basically an easy win.

        Show
        Sam Marshall added a comment - note: Eloy points out that it's possible to do is_array faster: "casting to array and === is like 8 times faster than doing is_array(), btw. if ((array)$arr === $arr" However, I don't think there is a value to this check any more - maybe it was needed once but after several versions worth of warnings, it should be good to go. We don't check every random function argument to see if it's an array. I checked this on core systems and found similar results - a small but measurable performance impact, basically an easy win.
        Hide
        Sam Marshall added a comment -

        Submitting for review.

        Show
        Sam Marshall added a comment - Submitting for review.
        Hide
        Dan Poltawski added a comment -

        +1, I agree with everything you said.

        Show
        Dan Poltawski added a comment - +1, I agree with everything you said.
        Hide
        Dan Poltawski added a comment -

        (This is sort of equivalent to our inner loop code, if we did it in C, it might be assembly )

        Show
        Dan Poltawski added a comment - (This is sort of equivalent to our inner loop code, if we did it in C, it might be assembly )
        Hide
        Dan Poltawski added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Eloy Lafuente (stronk7) added a comment -

        In this case, i think the change is ok and safe, more yet s(), that uses htmlspecialchars() surely will end throwing some warning itself...so.. ok.

        Show
        Eloy Lafuente (stronk7) added a comment - In this case, i think the change is ok and safe, more yet s(), that uses htmlspecialchars() surely will end throwing some warning itself...so.. ok.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated (24 & master), thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (24 & master), thanks!
        Hide
        Jason Fowler added a comment -

        Wow Sam... long, difficult test right there

        Passes nicely though.

        Show
        Jason Fowler added a comment - Wow Sam... long, difficult test right there Passes nicely though.
        Hide
        Sam Marshall added a comment -

        Jason: Sorry for creating so much work for testers.

        Show
        Sam Marshall added a comment - Jason: Sorry for creating so much work for testers.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Did you think this day was not going to arrive ever?

        Your patience has been rewarded, yay, sent upstream, thanks!

        Closing...ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Did you think this day was not going to arrive ever? Your patience has been rewarded, yay, sent upstream, thanks! Closing...ciao

          People

          • Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: