Last modified: 2014-09-24 01:12:10 UTC
Noticed on https://en.wikipedia.org/w/index.php?title=Special:RecentChanges&limit=500 with "EnhancedRecentChanges" preference enabled. Blocks Firefox 3.6.27. The browser pops up alerts about an unresponsive script every 10 to 20 seconds, so often and for so long that one invariably ends killing the script. Better in Chrome, but still takes 10 seconds to collapse all the entries with more than one edit. The problem seems to be that line 335 in jquery.makeCollapsible() http://svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/resources/jquery/jquery.makeCollapsible.js?view=markup#l335 causes lots of collapsing *with animations*. Not good. This line is supposed to initially collapse an element marked with class "collapsed", so it really should not do animations but instantly hide instead. Especially since it's called for each any every collapsible collapsed element. Furthermore, after inspecting that code, I think that in function toggleElement, all the "if ( $defaultToggle ) { ... " cases should also honor the instantHide flag. As written, the default toggle always uses animations, but that's exactly what ultimately gets triggered by line 335 (via line 191). Before line 335, I think you should set a flag on the toggle to indicate that this time, collapsing is to be instantaneous. Then in the toggleLink* handlers, read that flag (removing it from the toggle; subsequent invocations may animate) and pass it on to toggleElement. And inside toggleElement itself, make sure the instantHide flag is respected also for the $defaultToggle cases. That should about resolve this.
Turns out that the above idea to avoid animations works, but makeCollapsible() on FF 3.6.27 is still too slow. It still pops up the alert about an unresponsive script once. (At least not multiple times.)
Testing with a slightly rewritten makeCollapsible (first set up and collect all the elements to collapse, then in a second phase actually collapse them) gives me the following rough timing (FF 3.6.27, MacOS 10.6.8): about 10 seconds to collect and set up the collapsible tables, then another about 13 seconds to actually collapse them. FF setting dom.max_script_runtime is set to the default value of 10 seconds. Of course I could just increase this timeout, but that's not really a solution. Collapsing O(50) tables with O(200) rows really should not take that long. Nor should finding the tables and setting up the toggles.
Created attachment 10193 [details] Rewritten jquery.makeCollapsible Short version: the attached rewrite is between two to four times faster, but needs full testing. Long version: Analyzing this more, I think the main problems are that the current implementation: * uses classes to record collapsed/expanded states, * and collapses each of the initially collapsed elements individually, * and just triggers the installed click event handlers to collapse them. Hence many click handlers are invoked. Each one changes some classes, then hides the element. Changing classes may cause page reflows, hiding may inadverently use animations, and triggering the click events causes event bubbling plus other overhead that's neither needed nor wanted in this case. The worst appears to be mixing changing classes with hiding (because hiding stores the computed style, which has to be recalculated after each reflow after each class change), next worst are the animations. The attachment is a rewrite of jquery.makeCollapsible that tries to alleviate these problems without changing the interface. It still uses and changes classes, but it uses a two-phase algorithm for initialization: in a first phase, it just records all the modifications (elements on which classes need to be changed, elements to be hidden), and then in a second phase plays back these recorded changes (making extra sure to avoid animations) at the end. BTW, $.hide() internally uses a similar mechanism: they first record for all elements the computed style and then modify all the elements, instead of doing both steps individually for each element one after the other, as style modifications also may cause reflows. Further optimizations include: * Recording the changes in plain arrays (like data.toggles = []) instead of using initially empty jQuery objects (data.$toggles = $()). The point here is that if we used that and then data.$toggles = data.$toggles.add( $that ), each "add()" call would try to make sure that the resulting set contained only unique elements. We do know, however, that this check is completely superfluous in our case. Avoiding these jQuery.unique() calls that jQuery internally makes gives another significant speedup. This is also why the rewritten code doesn't use $.closest() to find the collapsible element from a toggle as that somehow also calls jQuery.unique(); instead collapsible belonging to a toggle is passed as a parameter (like it was already done for the custom toggles). * Using $.children ( 'foo' ) instead of $.find ( '> foo' ) wherever possible. I had the impression that that also is a bit faster. * Using a (new) function $.changeClass (fromClasses, toClasses) to "atomically" remove some classes and add some other classes instead of $.removeClass( fromClasses ).addClass ( toClasses ). That reduces the number of reflows for class changes by a factor of two. (The function is supposed to take two string arguments, both containing single class names or lists (whitespace-separated) of class names. The implementation does not, like addClass/removeClass, support functions as parameters.) * Not invoking the click handlers to collapse the initially collapsed elements. Even using $.triggerHandler() instead of $.trigger() still had significant unwanted overhead; remembering and then calling the appropriate toggle* function directly is still noticeably faster. The result is still not lightning fast, but in my (admittedly somewhat ad-hoc) tests significantly faster than the current implementation. Of my FF 3.6.27, it's about three to four times faster than the current implementation (fast enough to avoid the "unresponsive script" alerts with the default setting of dom.max_script_runtime); on Chrome, the speedup is about a factor of two. Note that I did test this implementation only manually on Special:Recentchanges; full testing exercising all cases (custom toggles, other stuff) still needs to be done. Testing on Special:Recentchanges actually exercises only the "table" and toggleLinkPremade() paths. The code would also need a unit test for $.changeClass(). Maybe that should even be split out in its own jquery plugin. The rewrite should be functionally identical to the currently existing implementation, with one exception: the new implementation of $.makeCollapsible() returns the subset of elements that were enabled, whereas the current implementation returns the full set passed in.
I forgot: the attachment is not a patch but the full JS file. I touched just about everything in that plugin anyway, so a patch wouldn't have made much sense.
Thanks, will check it out soon!
Created attachment 10198 [details] Patch for further improvement after r112457 The .addClass ( 'mw-collapsible mw-made-collapsible' ) also is rather expensive, probably because it causes reflows of pretty much the whole page. I see that in r112457 you had already eliminated the need for the mw-made-collapsible class in CSS. Not using a class for this marker but storing it instead as data again helps avoid page reflows and brings even more speedup. The patch implements this (plus has an optimized getRows() function to use instead of .children( 'tbody' ).children ( 'tr:first' ), which incurs expensive Sizzle invocations). The patch does require r112457, though. As long as there's CSS that relies on that mw-made-collapsible class to display the toggle arrows, you can't avoid setting that class.
Created attachment 10200 [details] Patch for further improvement after r112754 Corrected patch.
Created attachment 10234 [details] Minor correction Patch fixes a minor oversight in the toggleLinkDefault operation. Patch created from attachment 10200 [details]. Same correction is also to be applied to attachment 10193 [details] (the full file).
See also bug 35176.
And also bug 35257 (nested collapsible tables).
Raising priority re Lupo's two items
*** Bug 31945 has been marked as a duplicate of this bug. ***
What's the problem with toggling a few display="none" properties? We would like to use the mw-collapsible stuff (which I belief is based on the jQuery stuff discussed here) in the German Wikipedia but we can't because of the crappy, broken, distracting, time-wasting animations (also see bug 31832). Remove the animations please or make it opt-in (via class="mw-collapsible-animated") and all problems are gone.
Lupo, thanks for the patch. Are you interested in using developer access to directly suggest it into our Git source control system? https://www.mediawiki.org/wiki/Developer_access https://www.mediawiki.org/wiki/Git/Workflow#How_to_submit_a_patch
(In reply to comment #14) > Lupo, thanks for the patch. Are you interested in using developer access to > directly suggest it into our Git source control system? Sumana, I have developer access since 2012-04-04. (I didn't when I submitted the bug.) Anyway Krinkle said he'd take care of this, and I don't have the time in the forseeable future to really polish this, fully test it, write unit tests, and shepherd it through code review.
Hi Lupo, You're right in that I'm planning to tackle makeCollapsible. The current version was meant to be a basic collapserizerererer to get something standardized - since up until now nothing for this existed in MediaWiki core and the solutions the community has developed over time on various wikis didn't match each other and required somewhat complicated HTML structures. But as not uncommon with simple things, they get more complicated. And right now the jquery.makeCollapsible is in a state where it is getting hacky very fast as soon as the case is not "basic" anymore. I've looked at the rewrite you did and it looks pretty straight forward (I had something similar in mind, although I was planning to rewrite it from scratch). Unfortunately I don't have time for this on the short term either. So unless someone else takes it, I'll probably get back this in a few months and take on a rewrite, taking your code as the new foundation (You'll get full credit for it of course :) ).
(In reply to comment #16) > Unfortunately I don't have time for this on the short term either. So unless > someone else takes it, I'll probably get back this in a few months ... Too bad. It's not just a speed issue, but would also solve bug 35176 and bug 35257.
Can't you just take Lupo's patch for the moment? I too am really annoyed by bug 35176 and bug 35257. If you plan a clean rewrite anyway, it seems better to have a fix in the meanwhile instead of a broken mediawiki.
It's been a few months - no idea what the actual progress is, but could someone at least put in a temporary fix, per what EoD said? Consistency and proper fixes are good, but the thing is still broken now and having something that at least works in the meantime would really help folks on less fancy machines.
Working on this now. Short time improvements: - Re-using the same $collapsible jQuery object instead of re-creation of $(this) - Use .data instead of dom manipulation (className property) - Use $.nodeName( HTMLElement, tag ) to detect something is an <a>, instead of using creating a jQuery object and calling is.('a'), which goes through a lot of selector stuff. - Fix bug where it says it does instantHide but actually still triggers the event that causes initial animations.
Done: Idb9ca00c03ec7d70903ed7fd79e427efa270ace4
*** Bug 31832 has been marked as a duplicate of this bug. ***
If there are a lot of areas to init we can probably speed it up a bit by moving the content into a document fragment while we setup the collapsible elements. Would be interesting to have a jQuery method for that.
Not sure why this is closed. "mw-collapsible mw-collapsed" still delays everything when the page is loaded. It still creates the same horrible slow and most of the time broken animations. Example: http://de.wikipedia.org/wiki/Wikipedia:WikiProjekt_Vorlagen/Werkstatt?uselang=en#Fehlerhafte_Einbindungen_von_Vorlage:Infobox Expected behavior: Clicking "Collapse" should collapse the content. That's all. Just collapse. It should not reflow thousands of DOM elements thousands of times. This crappy animation blocks the page for several seconds and (especially on slower machines) the whole web browser. It should cause 1 reflow. One. Not thousands. My currently used browser is Opera 12.02.
(In reply to comment #24) > Not sure why this is closed. "mw-collapsible mw-collapsed" still delays > everything when the page is loaded. > http://de.wikipedia.org/wiki/Wikipedia:WikiProjekt_Vorlagen/Werkstatt?uselang=en#Fehlerhafte_Einbindungen_von_Vorlage:Infobox Cannot confirm with Firefox 16.0.1 on Fedora 16 and being logged in: When clicking "expand" or "collapse" next to ".arpa" the animation is quick (definitely way less than a second) and smooth for that table with 173 entries. Using Opera 12.02 build1578 on Fedora 16 without being logged in, it does take longer, maybe around one and a half second, and the animation is not smooth at all. It looks like this problem is specific to the Opera browser, hence setting "Web browser" field to "Opera".
The current version of Chrome also freezes entirely for about 10s before collapsing everything when I look at my watchlist. It's smoother than what happens in Opera, but if anything even more annoying due to the complete unresponsiveness during that time.
For the love of all things shiny, could someone please look into fixing this? Whatever was being used in 1.16 and 1.17 was fine or I never would have gotten used to having collapsible watchlists and whatnot in the first place, but now this is making wikis completely unusable for me unless I turn it off. Now I admit my computer is ten years old (and I'm using Opera because it's apparently the only thing that can even run in real time anymore on it), but having to wait over a minute (an improvement over 1.19, granted, which is hilarious because that's what Wikia is on and they have the collapsible things on by default for everyone) for a bloody recentchanges to collapse itself is completely ridiculous.
As this makes some pages impossible to read as per last comment, bumping severity.
So I was doing some debugging and. It looks like the culprit is code like this, repeated three times: if ( $defaultToggle ) { // Exclude tablerow containing togglelink $containers.not( $defaultToggle.closest( 'tr' ) ).stop(true, true).fadeOut(); } else { if ( options.instantHide ) { $containers.hide(); } else { $containers.stop( true, true ).fadeOut(); } } The instantHide check should be before the $defaultToggle check... (Just grep for "instantHide" to find the rest.)
Fiddling this plugin isn't going to result in appreciable gains for large numbers of elements, because, no matter *what* you do, .makeCollapsible() still has to iterate through all of the elements, one-by-one, and collapse them, and reflow the page, and do all this *after* $( document ).ready(), no less. This is unacceptable. The proper way of doing this is with creative CSS rules and the jQuery.live() handler (for attaching onclick events to toggle-elements), because CSS rules don't force *each* element to re-flow the entire page if they're loaded before the content. If using CSS is an unfeasible solution, you should at least stop using .makeCollapsible() for watchlists and recentchanges lists; older versions of mediawiki didn't use it, and it's a simple thing to change.
I submitted Gerrit change #34971 to fix the issue I mentioned in comment 29. Nevertheless bookraiders might be right, although I'm not convinced that such desperate measures will be needed.
(In reply to comment #30) > Fiddling this plugin isn't going to result in appreciable gains for large > numbers of elements, because, no matter *what* you do, .makeCollapsible() still > has to iterate through all of the elements, one-by-one, and collapse them, and > reflow the page, and do all this *after* $( document ).ready(), no less. This > is unacceptable. The proper way of doing this is with creative CSS rules and This is exactly what I've been talking about. Instead of doing the hiding with inline styles and animations, use CSS classes (and animations with CSS3 transitions, falling back to simple instant toggles). That way the initial hide is natural and automatic without even the slightest blink as mw-collapsed would already be display none from the get-go. I've tried implementing this a few times though, and it wasn't as straight forward. Especially with making it transition from display: none; to display: block vertically (overflow hidden, height: 0, and then forcing display block for non-block elements like lists and tables). The javascript toggles would merely toggle the classes, no more than that. > should at least stop using .makeCollapsible() for watchlists and recentchanges > lists; older versions of mediawiki didn't use it, and it's a simple thing to > change. It is only used on the "js-enhanced" version of the changes lists (which is disabled by default and can be enabled by the user through their preferences). And this enhanced version always used javascript for toggling. iirc, the old versions were even worse.
> It is only used on the "js-enhanced" version of the changes lists (which is disabled by default and can be enabled by the user through their preferences). Ok, fair point, but some wikis enable that be default, even for logged-out users. Seems stupid, yes, but still. Also, enhanced-recentchanges, even with javascript turned off, is still useful more useful than the default (which merely sorts edits by timestamp). > this enhanced version always used javascript for toggling. iirc, the old versions were even worse. For what it's worth, I fiddled a couple of files in the core of mediawiki to come up with a version of enhanced recent changes which uses CSS, rather than .makeCollapsible(). I've tested it, and it works instantaneously. Unfortunately, I'm not a mediawiki developer, and the process for getting developer access seems rather involved.
(In reply to comment #33) > For what it's worth, I fiddled a couple of files in the core of mediawiki to > come up with a version of enhanced recent changes which uses CSS, rather than > .makeCollapsible(). I've tested it, and it works instantaneously. > Unfortunately, I'm not a mediawiki developer, and the process for getting > developer access seems rather involved. I'm sorry that that is what it seems to you. We've actually done a lot of work to make this easier, which was possible because we migrated from a high-end trust only access to Subversion, to a conversion to Git with a pre-review model. Now anyone can get access within hours and be submitting patches (kind of like Pull requests on GitHub). See https://www.mediawiki.org/wiki/Developer_access If you don't want to do that, I'd recommend you create a topic branch locally in your git clone of mediawiki core (git branch makecollapsible --track origin/master; git checkout makecollapsible). Then make your changes and commit it locally. Then you can use `git show --pretty=email` or `git format-patch ..` to create a patch, output it to a file and attach that to this bugzilla ticket. Then someone else will put it in the review queue for you (keeping your name as the author).
I've never used git, so that's why it seems involved to me. :)
bookraiders, we're happy to help you get started -- come into our chat channel anytime https://www.mediawiki.org/wiki/MediaWiki_on_IRC or watch https://commons.wikimedia.org/wiki/File:Berlin_Hackathon_2012_-_Using_Git_and_Gerrit_%28Patrick_Reilly%29.ogv if you prefer video. I've requested a few more reviews on Bartosz's Gerrit change #34971 to try to fix the immediate problem.
My changeset (Gerrit change #34971 / I13313ec4) was merged. This might help alleviate this issue a little; let's wait for the deployment before either marking this as resolved or working on it further.
Splitting the part about enhanced watchlist/recentchanges to separate bug 51749 because this is both the most important part of this bug and should be particularly easy to work around (while the general solution is somewhere between "very hard" and "impossible"). Adjusting severity/priority accordingly.
