Moodle
  1. Moodle
  2. MDL-36548

Poor performance (can trigger slow script warnings) in file picker detail (list) view

    Details

    • Testing Instructions:
      Hide

      TEST THE FILEPICKER IN ALL SUPPORTED BROWSERS (explore)

      • Add a file resource
      • Click the "Add..." button in the file manager to launch the file picker
      • In the Server Files browser, navigate to a path that contains many files and/or folders, such as an imported "Legacy course files" folder
      • Switch between the three views - icons, detail (list), tree - and confirm that it switches to detail (list) view quickly and without triggering any slow script warnings
      • From the detail (list) view, navigate around and choose a file
      • Save the file resource to confirm that it works as expected

      It may be helpful to try this before and after applying the patch, to get a feel for the difference in performance.

      Show
      TEST THE FILEPICKER IN ALL SUPPORTED BROWSERS (explore) Add a file resource Click the "Add..." button in the file manager to launch the file picker In the Server Files browser, navigate to a path that contains many files and/or folders, such as an imported "Legacy course files" folder Switch between the three views - icons, detail (list), tree - and confirm that it switches to detail (list) view quickly and without triggering any slow script warnings From the detail (list) view, navigate around and choose a file Save the file resource to confirm that it works as expected It may be helpful to try this before and after applying the patch, to get a feel for the difference in performance.
    • Workaround:
      Hide

      Don't use the detail (list) view.

      Show
      Don't use the detail (list) view.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36548_master
    • Rank:
      46019

      Description

      When a large number of files and/or folders are being displayed in the file picker, the detail (list) view is very slow (compared to icon and tree views) - to the point that it will trigger slow script warnings in some cases.

      This is due to the way that the data is added to the YUI DataTable - it's being added using the addRows() function after the DataTable is initialised. Providing the data when initialising the DataTable seems to alleviate this issue - the YUI DataTable is much more efficient at handling initial data than at adding rows post-initialisation, perhaps because it doesn't expect to be given entire large datasets via addRows().

        Activity

        Hide
        Andrew Nicols added a comment -

        Thanks for looking at this Paul - your help is very much appreciated. Let me know when it's ready for Peer Review and I'll take a look at your patch.

        Andrew

        Show
        Andrew Nicols added a comment - Thanks for looking at this Paul - your help is very much appreciated. Let me know when it's ready for Peer Review and I'll take a look at your patch. Andrew
        Hide
        Andrew Nicols added a comment -

        Ack sorry - hadn't seen that this was up for peer review. I'll try and take a look today.

        Show
        Andrew Nicols added a comment - Ack sorry - hadn't seen that this was up for peer review. I'll try and take a look today.
        Hide
        Andrew Nicols added a comment -

        Paul,

        This looks good and makes sense.

        [N] Syntax
        [Y] Output
        [Y] Whitespace
        [-] Language
        [-] Databases
        [Y] Testing
        [-] Security
        [-] Documentation
        [Y] Git
        [Y] Sanity check

        Syntax

        This is just a minor thing (and I know it's copied from the old code) but you're running:

        var parentid = scope.one('.'+classname).get('id');
        scope.tableview.render('#'+parentid);
        

        The DataTable.render() function then uses the provided argument and passes it to Node.one(). It would be slightly more efficient to pass the Node rather as Node.one() will just return the Node without re-selecting it.

        E.g.:

        var parentnode = scope.one('.' + classname);
        scope.tableview.render(parentnode);
        

        Or maybe even:

        scope.tableview.render(scope.one('.' + classname));
        

        Otherwise this is good to go up for Integration Review.

        Thanks again for your efforts on this one,

        Andrew

        Show
        Andrew Nicols added a comment - Paul, This looks good and makes sense. [N] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Syntax This is just a minor thing (and I know it's copied from the old code) but you're running: var parentid = scope.one('.'+classname).get('id'); scope.tableview.render('#'+parentid); The DataTable.render() function then uses the provided argument and passes it to Node.one(). It would be slightly more efficient to pass the Node rather as Node.one() will just return the Node without re-selecting it. E.g.: var parentnode = scope.one('.' + classname); scope.tableview.render(parentnode); Or maybe even: scope.tableview.render(scope.one('.' + classname)); Otherwise this is good to go up for Integration Review. Thanks again for your efforts on this one, Andrew
        Hide
        Paul Nicholls added a comment -

        Hi Andrew,
        I've pushed another commit to each of the branches which makes the change you suggested. I hadn't bothered with it earlier, as I was trying to make the minimal change for maximal improvement - but I guess we may as well go that small step further

        -Paul

        Show
        Paul Nicholls added a comment - Hi Andrew, I've pushed another commit to each of the branches which makes the change you suggested. I hadn't bothered with it earlier, as I was trying to make the minimal change for maximal improvement - but I guess we may as well go that small step further -Paul
        Hide
        Andrew Nicols added a comment -

        Hi Paul,

        That's perfect. Of you can squash them into a single commit, I'll put this in for integration review in the morning.

        Many thanks,

        andrew

        Show
        Andrew Nicols added a comment - Hi Paul, That's perfect. Of you can squash them into a single commit, I'll put this in for integration review in the morning. Many thanks, andrew
        Hide
        Paul Nicholls added a comment -

        Hi Andrew,
        I've squashed the commits on both branches, so there's now just the one commit on each.

        -Paul

        Show
        Paul Nicholls added a comment - Hi Andrew, I've squashed the commits on both branches, so there's now just the one commit on each. -Paul
        Hide
        Dan Poltawski added a comment -

        Integrated (23 and master), thanks guys.

        This needs some good testing for rgressions.

        Show
        Dan Poltawski added a comment - Integrated (23 and master), thanks guys. This needs some good testing for rgressions.
        Hide
        Petr Škoda added a comment -

        I did not see any major perf improvements, maybe I did not have enough files, I tested it with the contents of our lib dir.

        Works fine in both branches using IE9, FF, Safari and Chrome.

        Thanks

        Show
        Petr Škoda added a comment - I did not see any major perf improvements, maybe I did not have enough files, I tested it with the contents of our lib dir. Works fine in both branches using IE9, FF, Safari and Chrome. Thanks
        Hide
        Dan Poltawski added a comment -

        Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week!

        ciao
        Dan

        Show
        Dan Poltawski added a comment - Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week! ciao Dan

          People

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

            Dates

            • Created:
              Updated:
              Resolved: