Last modified: 2013-09-20 06:46:27 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 50836 - Loading a page which requires many fonts causes high CPU load
Loading a page which requires many fonts causes high CPU load
Status: RESOLVED FIXED
Product: MediaWiki extensions
Classification: Unclassified
UniversalLanguageSelector (Other open bugs)
unspecified
All All
: High major with 1 vote (vote)
: ---
Assigned To: Amir E. Aharoni
https://en.wikipedia.org/w/index.php?...
: javascript, performance
: 51070 (view as bug list)
Depends on:
Blocks: 53015
  Show dependency treegraph
 
Reported: 2013-07-05 20:40 UTC by kipod
Modified: 2013-09-20 06:46 UTC (History)
19 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description kipod 2013-07-05 20:40:31 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.
Comment 1 kipod 2013-07-05 21:09:52 UTC
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.
Comment 2 kipod 2013-07-05 21:21:09 UTC
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.
Comment 3 John Mark Vandenberg 2013-07-08 07:03:52 UTC
Why was this set back to UNCONFIRMED?  Lots of people have experienced this, and kipod has even identified the fix.
Comment 4 Derk-Jan Hartman 2013-07-09 21:08:37 UTC
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
Comment 5 Ori Livneh 2013-07-10 00:01:04 UTC
(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.
Comment 6 kipod 2013-07-10 06:19:37 UTC
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.
Comment 7 kipod 2013-07-10 06:28:40 UTC
(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.
Comment 8 SpeedyGonsales 2013-07-10 06:59:29 UTC
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.
Comment 9 Andre Klapper 2013-07-10 09:06:17 UTC
(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.
Comment 10 Eduard Braun 2013-07-10 09:21:20 UTC
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
Comment 11 Andre Klapper 2013-07-10 10:34:59 UTC
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.
Comment 13 Bartosz Dziewoński 2013-07-10 11:24:30 UTC
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
Comment 14 kipod 2013-07-10 13:14:20 UTC
(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.
Comment 15 Gerrit Notification Bot 2013-07-10 15:27:24 UTC
Change 72967 had a related patch set uploaded by Amire80:
Update jquery.webfonts from upstream

https://gerrit.wikimedia.org/r/72967
Comment 16 Gerrit Notification Bot 2013-07-10 15:45:00 UTC
Change 72967 merged by jenkins-bot:
Update jquery.webfonts from upstream

https://gerrit.wikimedia.org/r/72967
Comment 17 Niklas Laxström 2013-07-10 16:12:59 UTC
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.
Comment 18 Nemo 2013-07-10 22:29:44 UTC
*** Bug 51070 has been marked as a duplicate of this bug. ***
Comment 19 Michael M. 2013-07-12 07:53:37 UTC
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.
Comment 20 kipod 2013-07-19 15:55:08 UTC
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.
Comment 21 Amir E. Aharoni 2013-07-19 15:57:25 UTC
It's resolved technically, but we want to write proper documentation and tests for the fix. Assigning to myself.
Comment 22 Siebrand Mazeland 2013-08-06 07:39:13 UTC
What's the progress on this, Amir?
Comment 23 Nemo 2013-09-20 06:46:27 UTC
(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.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links