Description
David Nind
2023-01-20 19:22:36 UTC
Created attachment 161832 [details] [review] Bug 32693: Proof of concept This is a PoC for discussion. Now, only the tab being viewed is processed and loaded. Thanks Pedro! I was keen to test your PoC, but got this when trying to apply the patch: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 32693: Proof of concept .git/rebase-apply/patch:65: trailing whitespace. error: sha1 information is lacking or useless (about.pl). error: could not build fake ancestor Patch failed at 0001 Bug 32693: Proof of concept Created attachment 161881 [details] [review] Bug 32693: Proof of concept This is a PoC for discussion. Now, only the tab being viewed is processed and loaded. (In reply to David Nind from comment #2) > Thanks Pedro! > > I was keen to test your PoC, but got this when trying to apply the patch: > > Apply? [(y)es, (n)o, (i)nteractive] y > Applying: Bug 32693: Proof of concept > .git/rebase-apply/patch:65: trailing whitespace. > > error: sha1 information is lacking or useless (about.pl). > error: could not build fake ancestor > Patch failed at 0001 Bug 32693: Proof of concept Apologies, this was written on top of other work unintentionally. I've rebased and should apply nicely now. Created attachment 161883 [details] [review] Bug 32693: Proof of concept This is a PoC for discussion. Now, only the tab being viewed is processed and loaded. Notes from testing (using KTD) ============================== I would say this makes a really good difference - apart from the System information tab, every other tab feels faster to load. I'm assuming for the system information tab that there is a lot to check, and this will depend on the Koha instance configuration. Before the patch ~~~~~~~~~~~~~~~~ - Initial: 19 requests 1.23 MB / 59.46 kB transferred Finish: 3.77 s DOMContentLoaded: 3.75 s load: 3.75 s - Reload (go somewhere else, such as the home page, then go back to the Abut Koha page): 18 requests 875.75 kB / 59.45 kB transferred Finish: 1.28 s DOMContentLoaded: 1.26 s load: 1.27 s - Refresh the page (Shift + Refresh button): 30 requests 1.78 MB / 389.59 kB transferred Finish: 1.38 s DOMContentLoaded: 1.37 s load: 1.39 s Note: With a refresh, a recurring error is (Bug 34973): downloadable font: Glyph bbox was incorrect (glyph ids 1 2 3 .......) (font-family: "Font Awesome 6 Free" style:normal weight:900 stretch:100 src index:0) source: http://127.0.0.1:8081/intranet-tmpl/lib/fontawesome/webfonts/fa-solid-900.woff2 After applying the PoC patch ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ (plus a flush_memcached, restart_all, and a fresh browser) Server information tab: - Initial: 19 requests 382.89 kB / 6.41 kB transferred Finish: 636 ms DOMContentLoaded: 612 ms load: 613 ms - Reload: 19 requests 165.26 kB / 29.78 kB transferred Finish: 653 ms DOMContentLoaded: 643 ms load: 652 ms - Refresh: 29 requests 891.46 kB / 336.53 kB transferred Finish: 603 ms DOMContentLoaded: 593 ms load: 610 ms Perl modules tab: - Initial: 18 requests 514.49 kB / 10.18 kB transferred Finish: 1.12 s DOMContentLoaded: 1.12 s load: 1.12 s - Reload: 18 requests 215.33 kB / 10.18 kB transferred Finish: 528 ms DOMContentLoaded: 519 ms load: 521 ms - Refresh: 30 requests 1.12 MB / 340.32 kB transferred Finish: 579 ms DOMContentLoaded: 564 ms load: 582 ms System information tab - Initial: 18 requests 28.45 kB / 5.70 kB transferred Finish: 3.13 s DOMContentLoaded: 3.11 s load: 3.11 s - Reload: 18 requests 28.45 kB / 5.70 kB transferred Finish: 1.17 s DOMContentLoaded: 1.16 s load: 1.16 s - Refresh: 30 requests 936.88 kB / 335.83 kB transferred Finish: 1.37 s DOMContentLoaded: 1.34 s load: 1.37 s (One warning in default KTD: Warning System preference 'RequireChoosingExistingAuthority' is disabled, but needs 'AutoCreateAuthorites' enabled. Otherwise catalogers can add headings that will not be linked to authorities) Koha team tab: - Initial: 18 requests 127.34 kB / 18.85 kB transferred Finish: 577 ms DOMContentLoaded: 573 ms load: 598 ms - Reload: 18 requests 127.34 kB / 18.86 kB transferred Finish: 632 ms DOMContentLoaded: 627 ms load: 630 ms - Refresh: 30 requests 1.04 MB / 349 kB transferred Finish: 664 ms DOMContentLoaded: 621 ms load: 672 ms Licenses tab: - Initial: 18 requests 41.74 kB / 8.21 kB transferred Finish: 510 ms DOMContentLoaded: 487 ms load: 494 ms - Reload: 18 requests 41.74 kB / 8.21 kB transferred Finish: 511 ms DOMContentLoaded: 494 ms load: 495 ms - Refresh: 30 requests 950.17 kB / 338.34 kB transferred Finish: 522 ms DOMContentLoaded: 506 ms load: 527 ms Translations tab: - Initial: 18 requests 31.09 kB / 7.35 kB transferred Finish: 497 ms DOMContentLoaded: 492 ms load: 493 ms - Reload: 18 requests 31.09 kB / 7.35 kB transferred Finish: 499 ms DOMContentLoaded: 496 ms load: 497 ms - Refresh: 29 requests 889.17 kB / 337.48 kB transferred Finish: 510 ms DOMContentLoaded: 504 ms load: 516 ms Timeline tab: - Initial: 18 requests 554.09 kB / 35.05 kB transferred Finish: 591 ms DOMContentLoaded: 563 ms load: 591 ms - Reload: 18 requests 554.09 kB / 35.06 kB transferred Finish: 576 ms DOMContentLoaded: 547 ms load: 574 ms - Refresh: 30 requests 1.46 MB / 365.19 kB transferred Finish: 606 ms DOMContentLoaded: 604 ms load: 613 ms Dedications tab: - Initial: 19 requests 159.28 kB / 29.19 kB transferred Finish: 509 ms DOMContentLoaded: 501 ms load: 508 ms - Reload: 18 requests 27.39 kB / 5.82 kB transferred Finish: 487 ms DOMContentLoaded: 473 ms load: 474 ms - Refresh: 30 requests 935.83 kB / 335.96 kB transferred Finish: 518 ms DOMContentLoaded: 504 ms load: 524 ms David, that's great, thank you so much for this analysis. I think the biggest improvement for me here is the much shorter time to first render. With this patch, whenever you click the "About" main menu item (default server information tab), it's no longer a slog. If others agree with this approach (Katrin, Martin, Tomas, Joubu?), I'll pick this back up to finish and tidy up the patch for submission. If not, or there are other better alternative suggestions, please do say. This is a great improvement; It's something I've always intended to do but never hit the top of the pile. Yes please, do the final polishing you see fit Pedro, I think this, and the subsequent bugs that apply on top will really help system administrators with spotted issues. Created attachment 162208 [details] [review] Bug 32693: Update tab_item html helper tab_item html helper now allows for a boolean tablink option to set if the the tab item is a link or not Created attachment 162209 [details] [review] Bug 32693: Update tab_items tab_items are now linktabs tab_items are now styled active depending if the URL param 'tab' matches the tab name or not Created attachment 162210 [details] [review] Bug 32693: Update tab_panels Each tab panel's content has now been moved to its own BLOCK. Each tab panel's content is now only loaded if said tab is the one being viewed Created attachment 162211 [details] [review] Bug 32693: Preparation: About script: docdir docdir is used in both team + history tabs was previously being loaded in team and additionally used in history but because now we're only loading one tab at a time, we need to move this Created attachment 162212 [details] [review] Bug 32693: Preparation: About script: Move template params These template params are only used in the default first 'about' tab but were being added further down in the script Created attachment 162213 [details] [review] Bug 32693: Preparation: About script: versions versions is being used in both 'team' and 'about' tabs, needs to be moved Created attachment 162214 [details] [review] Bug 32693: About script: Process only the tab being queried Created attachment 162219 [details] [review] Bug 32693: Update tab_item html helper tab_item html helper now allows for a boolean tablink option to set if the the tab item is a link or not Signed-off-by: David Nind <david@davidnind.com> Created attachment 162220 [details] [review] Bug 32693: Update tab_items tab_items are now linktabs tab_items are now styled active depending if the URL param 'tab' matches the tab name or not Signed-off-by: David Nind <david@davidnind.com> Created attachment 162221 [details] [review] Bug 32693: Update tab_panels Each tab panel's content has now been moved to its own BLOCK. Each tab panel's content is now only loaded if said tab is the one being viewed Signed-off-by: David Nind <david@davidnind.com> Created attachment 162222 [details] [review] Bug 32693: Preparation: About script: docdir docdir is used in both team + history tabs was previously being loaded in team and additionally used in history but because now we're only loading one tab at a time, we need to move this Signed-off-by: David Nind <david@davidnind.com> Created attachment 162223 [details] [review] Bug 32693: Preparation: About script: Move template params These template params are only used in the default first 'about' tab but were being added further down in the script Signed-off-by: David Nind <david@davidnind.com> Created attachment 162224 [details] [review] Bug 32693: Preparation: About script: versions versions is being used in both 'team' and 'about' tabs, needs to be moved Signed-off-by: David Nind <david@davidnind.com> Created attachment 162225 [details] [review] Bug 32693: About script: Process only the tab being queried Signed-off-by: David Nind <david@davidnind.com> Have signed off. I hope this was reading for testing! I repeated the timings from comment #6, and things seem to load even faster (particularly the system information tab: 2.37s, 599ms, and 970ms respectively). All the tabs look OK and display as expected for me (tested using Firefox and Chromium). Improving the page titles and breadcrumb would be a bonus (instead of just 'About Koha'), but I think that could be a separate bug - happy to create it. About is slow. But this feels much more as enh than bugfix. Correct me if I am mistaken.. (In reply to Marcel de Rooy from comment #24) > About is slow. But this feels much more as enh than bugfix. Correct me if I > am mistaken.. Agreed. Yes. Created attachment 162660 [details] [review] Bug 32693: Update tab_item html helper tab_item html helper now allows for a boolean tablink option to set if the the tab item is a link or not Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 162661 [details] [review] Bug 32693: Update tab_items tab_items are now linktabs tab_items are now styled active depending if the URL param 'tab' matches the tab name or not Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 162662 [details] [review] Bug 32693: Update tab_panels Each tab panel's content has now been moved to its own BLOCK. Each tab panel's content is now only loaded if said tab is the one being viewed Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 162663 [details] [review] Bug 32693: Preparation: About script: docdir docdir is used in both team + history tabs was previously being loaded in team and additionally used in history but because now we're only loading one tab at a time, we need to move this Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 162664 [details] [review] Bug 32693: Preparation: About script: Move template params These template params are only used in the default first 'about' tab but were being added further down in the script Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 162665 [details] [review] Bug 32693: Preparation: About script: versions versions is being used in both 'team' and 'about' tabs, needs to be moved Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 162666 [details] [review] Bug 32693: About script: Process only the tab being queried Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Can you please rebase? Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 32693: Update tab_item html helper Applying: Bug 32693: Update tab_items Applying: Bug 32693: Update tab_panels Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/en/modules/about.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/about.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/about.tt error: Failed to merge in the changes. Patch failed at 0001 Bug 32693: Update tab_panels hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-32693-Update-tabpanels-_ghrc9c6.patch Created attachment 165251 [details] [review] Bug 32693: Update tab_item html helper tab_item html helper now allows for a boolean tablink option to set if the the tab item is a link or not Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 165252 [details] [review] Bug 32693: Update tab_items tab_items are now linktabs tab_items are now styled active depending if the URL param 'tab' matches the tab name or not Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 165253 [details] [review] Bug 32693: Update tab_panels Each tab panel's content has now been moved to its own BLOCK. Each tab panel's content is now only loaded if said tab is the one being viewed Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 165254 [details] [review] Bug 32693: Preparation: About script: docdir docdir is used in both team + history tabs was previously being loaded in team and additionally used in history but because now we're only loading one tab at a time, we need to move this Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 165255 [details] [review] Bug 32693: Preparation: About script: Move template params These template params are only used in the default first 'about' tab but were being added further down in the script Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 165256 [details] [review] Bug 32693: Preparation: About script: versions versions is being used in both 'team' and 'about' tabs, needs to be moved Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 165257 [details] [review] Bug 32693: About script: Process only the tab being queried Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Pushed for 24.05! Well done everyone, thank you! It broke t/db_dependent/www/history.t Created attachment 166421 [details] [review] Bug 32693: Fix history.t Follow-up for history.t pushed (soon). Not backported to 23.11.x No update to the Manual needed. |