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

      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.)

        Gliffy Diagrams

          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: