Last modified: 2013-09-20 06:46:27 UTC
see report here: [[en:Wikipedia:Village_pump_(technical)#Heavy_Javascript_load_after_loading_some_articles.]] (bottom of [[en:WP:VPT]] on July 5 2013. if you don't find it there, look in the archives). Initial debugging of the issue points to the function injectCSS(). as it turns out, ULS calls injectCSS() once for each required font, which, on some pages (methinks pages with many interwiki links, mainly) can be dozens of times. apparently, on some browsers injectCSS() can be expensive (maybe the browser re-renders the whole page, in light of the new stylesheet information?) and calling it dozens of times causes high CPU load. clearly, the right thing to do here is to collect all the different CSS bits and pieces you want to inject, and call injectCSS() exactly once. this piece of code demonstrate what some people already know: injecting stuff can be habit forming, and can be bad for your health, so you absolutely want to minimize it. (btw: the function itself is somewhat disgusting. how is it better than function injectCSS( css ) { $( '<style>' , {type: 'text/css', rel: 'stylesheet' } ) .text( css ) .appendTo ('head' ); } // really no need to return anything: the single callsite ignores // the return value anyway. anywho, you can prepend "return" // to make it behave more like existing code if you really want to... peace.
the function "load" is also asinine, and can cause a high load: it has two problems: one of them is using $.inArray() to test whether this fontface was already loaded: this means linear search on the array every single time (that is, for every single html element with either "lang", "style" or "class" attributes on the page!), and for pages with large number of elements, this can be significant. whats more, there is no reason not to add the font family to the list even when it's not found, to save us some work with future elements with same font family. even worse, when getCSS() returns "false" (which it does for 99% of the elemnts, e.g. when fontFamily is sans-serif), we don't mark this font family as "tested", so when we find the next element with sans-serif, we do all this work again. from performance POV, this is atrocious. here is a better version of load() - 100% untested. load: function( fontFamily ) { if ( this.fonts[fontFamily] ) return this.fonts[fontFamily] != "abnormal"; // does the font have "normal" face? var styleString = this.getCSS( fontFamily, 'normal' ); if ( styleString ) { injectCSS( styleString ); this.fonts[fontFamily] = true; return true; } // Font does not have "normal" face. this.fonts[fontFamily] = "abnormal"; return false; } note that "this.fonts" here is an object rather than an array (to save the linear searches), so it should be initialized to {} rather than []. peace.
P.S: i used "abnormal" to signify that getCSS() returnd false, when called with this font family and variant "normal". i do not think this is a good name - maybe "empty" of "builtin" or something similar will be better, as long as one can be confident it's not a valid fontFamily name. it has to eveluate as boolean true, though, so you can;t use false or empty string. alternatively, false can work, as long as you modify the first line to if ( this.fonts.hasOwnProperty( fontFamily ) ) so the thing becomes load: function( fontFamily ) { if ( this.fonts.hasOwnProperty( fontFamily ) ) return this.fonts[fontFamily]; var styleString = this.getCSS( fontFamily, 'normal' ); if ( styleString ) { injectCSS( styleString ); return this.fonts[fontFamily] = true; } return this.fonts[fontFamily] = false; } (for brevity, assign and return on one line - don't do it in real life). peace.
Why was this set back to UNCONFIRMED? Lots of people have experienced this, and kipod has even identified the fix.
I've requested ULS be disabled, this has taken too long, it's annoying ppl without reason. https://bugzilla.wikimedia.org/show_bug.cgi?id=51070
(In reply to comment #1) > clearly, the right thing to do here is to collect all the different CSS bits > and pieces you want to inject, and call injectCSS() exactly once. I've only noted a single repaint in Chrome Dev Tools on https://en.wikipedia.org/wiki/Tower_of_babel, and I'm not sure yet if it's attributable to ULS. But any form of DOM access is expensive, so the recommendation above is entirely sound. > the function "load" is also asinine, and can cause a high load: > > it has two problems: one of them is using $.inArray() to test whether this > fontface was already loaded: this means linear search on the array every > single time Hash tables lookups are worse than linear searches for arrays of this size due to the cost of computing the hash and the relatively poorer locality. (In fact, modern JS engines don't even implement property lookups as hash tables, but I figure that was what you had in mind when you recommended hasOwnProperty instead of $.inArray). https://en.wikipedia.org/wiki/Tower_of_babel has four fontFamilies: "Amiri", "Akkadian", "AbyssinicaSIL", and "CharisSIL". I used this to profile object vs. array lookup: > var myArray = ["Amiri", "Akkadian", "AbyssinicaSIL", "CharisSIL"]; > console.time('$.inArray'); for(var i = 0; i < 10000; i++) $.inArray('abc', myArray); console.timeEnd('$.inArray'); $.inArray: 18.029ms > var myObj = {"Amiri": null, "Akkadian": null, "AbyssinicaSIL": null, "CharisSIL": null}; > console.time('hasOwnProperty'); for(var i = 0; i < 10000; i++) myObj.hasOwnProperty('abc'); console.timeEnd('hasOwnProperty'); hasOwnProperty: 13.161ms The difference per call works out to just under half of one millionth of a second, certainly not enough to warrant calling either approach 'asinine'. I am running a recent build of Chromium, but I'd wager the difference even for older browsers would not cross the threshold of significance. It is a bit bizarre that load() has to be called 877 times on that page, though. That should be investigated. ULS contains quite a lot of new JavaScript and CSS code, so it is reasonable to expect that a close scrutiny of the code will reveal some measure of duplicate work and various other performance problems. It's good that you've dug into the code and pinpointed some specific problems, but please be less adversarial. Measuring and optimizing client-side performance is hard enough to do when you aren't under attack; calling a developer's code "asinine" or "disgusting" is not likely to make it any easier.
i think you missed the main point: when we call "load" with a font family such that this.getCSS() returns false (e.g. "sans-serif" on my browser), we do not cache the result. calling getCSS() is orders of magnitude more expensive than the search, regardless of whether the search is done the right way (through hash) or the wrong way (through $.inArray). as to profiling: the real test is to have ~20 entries in the font table (like in [[en:Japan]]), and then measure how long does it take to look for a font _which is not in the list_, which is what happens today, thanks to the problem mentioned above. you'll see that the search takes significantly longer than it should. still, the main problem is not the inefficient search, but rather calling getCSS thousands (or tens of thousands - depends on the page content) of times instead of once per each font family present on the page. as to why we call "load" so many times: easy. set a breakpoint, and follow the call stack upward until you'll find the call that's inside an .each() loop. peace.
(In reply to comment #5) > ULS contains quite a lot of new JavaScript and CSS code, so it is reasonable > to > expect that a close scrutiny of the code will reveal some measure of > duplicate > work and various other performance problems. It's good that you've dug into > the > code and pinpointed some specific problems, but please be less adversarial. > Measuring and optimizing client-side performance is hard enough to do when > you > aren't under attack; calling a developer's code "asinine" or "disgusting" is > not likely to make it any easier. you are correct that my style is/was a bit abrasive, and for this i apologize. however, the ULS team demonstrated pretty bad behavior of its own: please review the way Siebard responded on bug 49935 . in addition, turning it on on many wikis before cleaning the very serious performance issues (and causing browsers to freeze _is_ very serious issue), and then arguing that this thing should not have a disable switch in user preference is just wrong. peace.
If you ask me having "disable switch" ("kill switch") in user preferences is a must for almost any new things in MediaWiki UI. If we are unable to alter closed OS (e.g. Windows or Mac) but are able to alter open source one (Linux), MediaWiki should be smart and offer "disable switches" for new things. ULS is a good idea, but the same way I like to have "kill switch" for VisualEditor :) ("Remove VisualEditor from the user interface" in Gadgets), I agree there should be one for ULS, period.
(In reply to comment #8) SpeedyGonsales: This bug report is specifically and only about "Loading a page which requires many fonts causes high CPU load". If you want to generally talk about "preferences for new things" or what MediaWiki should be, please do that on some forum or Village Pump. Thanks for your understanding.
Since one is ignored "on some forum or Village Pump" (*) one tries other ways to voice one's opinion. Is it surprising that people then try it on Bugzilla where one has at least slight chances a dev will peek at one's message an comment on them? * e.g. http://www.mediawiki.org/wiki/Thread:Talk:Universal_Language_Selector/Disabling_ULS or http://en.wikipedia.org/wiki/Wikipedia:Village_pump_%28technical%29#.22Opt_out.22_of_VE_needed_under_preferences
Eduard: I can understand frustration, but no need for rhetorical questions, plus "I wasn't heard in the right place, so I tried again in some other, wrong place" is not a good argument. Tools (like bugtrackers, mailing lists, Gerrit) serve purposes and have dedicated audiences. Bugzilla is a technical bugtracker for bug reports that refer to specific codebases. It's not for meta-level discussions on development policies, plus from experience in other open source projects I can assure you that the more comments unrelated to a bug there are in bug reports, the more devs will (understandably) also start ignoring bugmail and bug reports. I assume that's not wanted by anybody, and that's why I explained how to avoid that.
Also reported at pl.wp: https://pl.wikipedia.org/wiki/Wikipedia:Kawiarenka/Kwestie_techniczne#Ostrze.C5.BCenie:_Skrypt_nie_odpowiada
Just a note that the Language Engineering team responsible for ULS will be holding an office hour later today, at 17:00 UTC on #wikimedia-office (on Freenode): http://www.mail-archive.com/wikitech-l@lists.wikimedia.org/msg69718.html
(In reply to comment #9) > SpeedyGonsales: This bug report is specifically and only about "Loading a > page > which requires many fonts causes high CPU load". If you want to generally > talk > about "preferences for new things" or what MediaWiki should be, please do > that > on some forum or Village Pump. Thanks for your understanding. it is unfortunate that "some forum or village pump" where the people responsible for the content of wikimedia deployment are actually listening, does not exist. there is another bug on bugzilla ( bug 46306 ), requesting/demanding a kill switch for ULS. this bug was closed with "resolved/wontfix". yet another open bug ( bug 51070 ) is requesting/demanding to disable ULS on enwiki. this one was not closed with "wontfix", but could have been just as well. so complaining about these demands/requests spilling into other bug reports is somewhat unfair, IMO. peace.
Change 72967 had a related patch set uploaded by Amire80: Update jquery.webfonts from upstream https://gerrit.wikimedia.org/r/72967
Change 72967 merged by jenkins-bot: Update jquery.webfonts from upstream https://gerrit.wikimedia.org/r/72967
The above patch makes the webfonts code about 4x faster in my testing. While it is an improvement, it does not completely remove the problem. To go further we need to reconsider the logic instead of just optimizing the current code. This patch is already on beta labs, can be tested for example in http://en.wikipedia.beta.wmflabs.org/wiki/Barack_Obama and is planned for deployment around 1800 UTC today.
*** Bug 51070 has been marked as a duplicate of this bug. ***
What about using .eachAsync (https://git.wikimedia.org/blob/mediawiki%2Fcore.git/HEAD/resources%2Fjquery%2Fjquery.async.js) instead of the .each loop over $( '*[lang], [style], [class]' ) ? Perhaps this is not the solution for this bug, but for bug 49935, but I'm not going to read through all those comments, and I think the .eachAsync is worth a try.
Why is this still open? as far as i can see, the patch solved the issue. is there any report about any more problems here? is anyone expected to do anything more regarding this issue? if it's not closed now, is there any chance of this getting closed ever? peace.
It's resolved technically, but we want to write proper documentation and tests for the fix. Assigning to myself.
What's the progress on this, Amir?
(In reply to comment #21) > It's resolved technically, Closing this one then, > but we want to write proper documentation and > tests > for the fix. Assigning to myself. and split this to bug 54364, to clarify status.
