GNOME Bugzilla – Bug 541605
updateBraille() can take an unreasonable amount of time with certain pages in Firefox 3
Last modified: 2009-03-10 00:05:50 UTC
Please describe the problem: I am experiencing major delays (over two seconds) when navigating over a long list of links using the tab key. If the links are separated by spaces, navigation works well, but when dashes are used to separate them, the delay depends on the number of links on the line. Steps to reproduce: 1. Go to http://www.freebsoft.org/~cerha/test.html 2. Navigate by TAB over the links presented on this page Actual results: The first line with 3 links works fine. The second line with 7 links works a little slower. The third line with 26 links makes the navigation so slow, that it is practically unusable. The fourth line with spaces between the 26 links works perfect again. Expected results: Does this happen every time? Yes Other information: This happens with Firefox 3.0 and the latest SVN Orca.
Created attachment 114003 [details] Simple page demonstrating the problem.
Still haven't gotten to the bottom of it, but a couple of observations for "safe keeping": 1. Amazingly enough, the lag is not in our code for getting the current line. That's happening rather quickly, and we're caching the line so we're not constantly going back to rebuild it. (Phew!) :-) 2. The lag seems to be in updateBraille(). Some sample times, calculating from the start of the method to its completion for one press of Tab: 0.538316011429 0.917539834976 0.890393972397 0.939221143723 1.14940094948 <--- Yikes! 0.925982952118 1.47695803642 <--- Yikes! 0.931169033051 Notice that often we're looking at nearly a full second to update braille -- and in some cases more than that. :-( Bug 535178 is all about getting the context at the same time we're getting the line, so the first thing I'll do is see if the ultimate fix for that makes this issue go away. If it does not -- or even if it does -- it might be worth investigating if we should cache not just the objects on a line, but the contexts as well. Stay tuned!
"Notes to self" as I do more testing and start working on the Gecko updateBraille() refactor: * The difference between the line with dashes and the line with spaces is that in the line with spaces, we ignore the spaces. So given the line with spaces we do have a bunch of regions (the text preceding the link is one region, and each link is its own region), but not nearly as many as we have when the dashes are used. In the latter case, we have a region for the initial string, a region for the first link, a region for the first dash, a region for the second link, a region for the second dash, etc., etc. We can improve performance *a bit* by using the stored text/getting the context at the same time we get the line. HOWEVER: * Half of the excessively long time being spent is not spent in Gecko, but instead in braille.getAttributeMask(). In the case of the dashes, we keep looking at the same paragraph over and over and over and over again. :-( I suspect/assume that given a Writer line of text which contains a bunch of links separated by dashes, we have one region because the links are not distinct objects. If so, I wonder if the thing to try is to not have separate regions for each link. If that doesn't work for some reason, how can we avoid the repeated checking, checking, checking of the same paragraph text? (That's a rhetorical question unless someone happens to know the answer already; otherwise, I'll investigate further. I just don't want to lose this train of though. Premature senility is such a drag....;-) )
Mike's bug 549325 is another case of updateBraille() taking an EXTREMELY long time. In some cases up to 15 seconds! :-( <insert swear words here> Here's his report since it's a dup. ------------------------- This problem can be reproduced on any really large page such as a converted HTML book. This is the reason for the severity I set. To reproduce do the following: 1. Open the NLS talking book beta site. Note I'm not listing the URL here as I'm not sure if it is public. 2. log in 3. go to update your settings. 4. set your number of books displayed to 500. 5. return to the home page. 6. select a category it shouldn't matter which one. 7 after the list of available books loads try to arrow down through the page. You should notice that navigation is really sluggish. On two of my systems as you arrow through the page it can take several seconds to go from one line to the next. If you try say all on this site you will probably get to restart your desktop. ------------------------- In light of this find, this problem is now at the top of my list....
*** Bug 549325 has been marked as a duplicate of this bug. ***
Created attachment 117857 [details] [review] proof of concept (maybe an interim solution) In the case of the problem Mike reported, 100% of the delay is caused by braille.py's getLineInfo() which we call back to back via braille.setFocus() and braille.refresh(). In the case of Tomas' report, approximately 50% of the delay can be attributed to this same issue. Question is, what shall we do about it? Observations: 1. If we already supported selection and text attributes in Firefox (and hopefully we will soon), we'd need to get the attribute mask and do so in a timely fashion. But we don't yet support it, so this long delay is to get a mask we won't/can't use (yet). 2. Could/should we be caching the line info or mask somewhere? If we did that, the delay would be cut in half at least. I still need to hunt down exactly what's taking so long with the mask creation.... I'm calling this patch a proof of concept because it illustrates where the delay is. It makes the issue go away by making it possible to say, "please don't bother getting the mask" and then not bothering for Gecko. :-) We *could* check it in as an interim fix while I work out the real issues. (Perhaps there will be other cases where we don't want to bother getting the mask?? Or perhaps if we know we're going to get it via braille.refresh(), there's no point in bothering in braille.setFocus()??) Or we can nod and say "yup, that's where the problem is alright" while I work out the real issues. :-) Thoughts?
Created attachment 117865 [details] [review] proposed fix for *part* of the problem In Mike's test case, some (but not all) of the delay is due to checking all of the hyperlinks. Well, on a super large page, we might have a hyperlink with a start index in the thousands (like 20,000) which is far beyond the end offset of what we're displaying. If we just bail once the links fall outside of what we're displaying, we'll pick up some improvement. With this patch looking at the worst cases, updateBraille() goes from 14 seconds down to 7. Some quick testing hasn't shown any breakage w.r.t. OOo Writer links, but I'd want to look at that more closely to be sure. Will, what do you think about this change? Any reason why we'd want to not bail under these conditions?
Hi Joanie, I am on enforced leave at the moment and will be leaving the country tomorrow morning, :) but could not help myself wondering, (probably a total stab in the dark). Does revision 5 of patch to bug 523693, reduce the time taken on large pages? I am hoping this is the case because the calculations are only done once, (at the moment its done in setFocus(), then immediately followed by the same calculations in refresh() ). Unfortunately i didnt have time to finish implementing the RFE. Will resolve on return. Thank you
Jon, I'll take a look at your patch. Thanks! The problems/delays I'm seeing are sufficiently bad, however, that we need to get to their underlying cause (i.e. just once is still way too long). I just ran the oowriter and gtk-demo tests on the patch in comment #7. Nothing broke. I'll run them on the FF tests in a bit. (Going to set up a second account as described on the wiki -- and repeatedly suggested by the Willie. ;-)). In the meantime, Mike, please test it. Things will still take a while, but they should take a bit less. While you do that, I'll keep working at the overall problem. Thanks
As you say it still isn't really snappy but it is better. As every little bit helps I'd say check it in.
Looks like the attribute delay is a firefox bug. I added a bunch of debugging code into the default script's getTextAttributes. A single call to the accessible text interface's getAttributes() is taking 2 seconds to return. I'll file a bug, but it would be handy to have a working Accerciser first....
Created attachment 117891 [details] [review] a bit more fixing As I commented earlier, asking Firefox for the attributes in Mike's test case is taking 2 full seconds to return. Yikes! In light of this, and our goal to also provide access to text attributes, we should try to cache them. This is a first, and conservative attempt which helps with the braille delays. As an aside, it seems we're doing some back-to-back updateBraille() calls when the document frame contains the caret (i.e. as opposed to a paragraph or other object). That's next on my agenda to work out. In the meantime, accepting that these calls happen.... I did some comparisons of the worst cases in Mike's example. 1. Current state: Call 1: 16 seconds Call 2: 12 seconds 2. With the tweak I made to braille.py: Call 1: 9 seconds Call 2: 3.7 seconds 3. With the caching of attributes: Call 1: 7 seconds Call 2: 1.5 seconds So this is around a 3x improvement. I'll keep at it, but in the meantime, please test.
Jon, I just did some performance testing using your patch and Tomas' test case. (Mike's test case is on my dev box, which is currently running regression tests; I'll try it there in a bit). In the meantime, I'm not seeing a performance difference between the current Orca and Orca after having applied your patch. The follow times reflect the start-to-finish execution of updateBraille() in seconds: Current Patch Difference 0.14 0.16 0.02 0.06 0.06 0.00 0.06 0.06 0.00 0.25 0.23 -0.02 0.15 0.14 -0.01 0.16 0.18 0.02 0.16 0.17 0.01 0.14 0.15 0.01 0.13 0.18 0.04 0.14 0.14 0.00 1.15 1.08 -0.06 0.96 0.92 -0.05 0.90 1.00 0.09 0.91 1.01 0.09 0.92 0.90 -0.02 0.90 0.93 0.03 0.93 0.92 0.00 0.93 0.96 0.03 Average difference: 0.01 seconds
I was curious, so I just did a similar run of my latest patch on this bug using Tomas' test case: Current Patch Difference 0.14 0.15 0.01 0.06 0.04 -0.02 0.06 0.04 -0.02 0.25 0.18 -0.07 0.15 0.10 -0.05 0.16 0.10 -0.06 0.16 0.10 -0.06 0.14 0.09 -0.05 0.13 0.08 -0.05 0.14 0.10 -0.04 1.15 0.36 -0.79 0.96 0.23 -0.73 0.90 0.20 -0.70 0.91 0.23 -0.68 0.92 0.23 -0.69 0.90 0.27 -0.63 0.93 0.22 -0.71 0.93 0.23 -0.70 Average difference: -0.33 seconds I'd still like to get that time down further. updateBraille() taking a quarter of a second sure beats it taking a full second; but a quarter of a second is still quite a bit -- and we can do better. BTW, the regression tests just completed. Green light on this patch.
I just ran the numbers on Mike's test case, being very systematic to ensure accurate comparisons. Jon, again, no difference I'm afraid. It could easily be that to take advantage of your changes, some modification will be needed in the Gecko updateBraille(). When you get back, perhaps we can chat about it and I'll better have my head wrapped around your patch by then. In the meantime.... Current Patch Difference 00.30 0.30 00.00 00.03 0.03 00.00 00.03 0.03 00.00 00.04 0.04 00.00 00.27 0.32 00.05 00.03 0.03 00.00 00.09 0.09 00.00 00.03 0.03 00.00 12.58 3.01 -09.56 <--- Yea! :-) 11.68 1.84 -09.84 <--- Yea! :-) 00.05 0.05 00.00 00.04 0.04 00.00 00.04 0.03 00.00 00.09 0.05 -00.04 00.11 0.10 -00.02 00.04 0.03 -00.01 13.82 2.53 -11.29 <--- Yea! :-) 11.38 1.79 -09.59 <--- Yea! :-) 00.02 0.02 00.00 00.04 0.04 00.00 00.04 0.04 00.00 00.04 0.03 -00.01 00.02 0.02 00.00 Average difference -1.75 seconds If I do say so myself, that doesn't suck all that badly. ;-) The act of getting attributes, as mentioned before, takes up to a couple of seconds when doing it for the behemoth document frame that is Mike's reading list. <smile> The offensively large times remaining fall in that ballpark -- and they're no longer in the ballpark of 12 seconds. I'll try to touch base with Marco and/or Surkov tomorrow and see what they think.
This is getting better. Here is another example of really bad performance on this site. To reproduce do the following: 1. search a category which has several hundred books. 2. press ctrl+end to move to the bottom of the page. 3. attempt to up arrow several times. for me it can take up to 15 seconds to get any speech.
Thanks Mike. Like I said, I'll keep tweaking. But I could probably tweak forever. So I'm wondering if it's worth checking this in for now so that we see the performance benefits it does provide? (i.e. does this patch break anything for you?).
This doesn't break anything so I think checking it in is a good idea.
Thanks Mike! Committed to trunk. I'll look at the issue you raised later this evening. If it's an updateBraille() issue, my money is on the inverse of the problem with links addressed in the patch I just committed, i.e.that we're at the bottom, working our way through 20,000 hyperlinks to get to our present location). If so, I have a fix in mind. If it's something totally different, well, we'll go from there. <smile> Obsoleting the proof of concept patch.
Created attachment 117981 [details] [review] try to quickly locate the relevant link range My suspicions (see previous comment) were correct. This patch is a variation on the last one. Before going through the links making the link-related mask, it tries to quickly locate the relevant link range. Cons: * Doesn't seem very pythonic/elegant Pros: * Doesn't seem to break Writer link support * Seems to solve Mike's last reported issue (keeping in mind the fact that Firefox is taking multiple seconds to respond to our asking for the attributes, and that there are changes we could -- and will -- be making to updateBraille() aka the refactor I had started. Will, can you come up with a better/more elegant way to accomplish the same thing? If so, I will gladly take it. :-) Thanks!
In terms of the attribute issue, I've filed a Mozilla Core bug: https://bugzilla.mozilla.org/show_bug.cgi?id=453605 text.getAttributes() can take an extraordinarily long time to return given a sufficiently large text object. Our blocking/tracking bug is bug #550800. In light of this, what remains to do on this particular bug is deal with the issue Mike raised in Comment #16. The issue in the original report can still be improved. That is part of bug 535178.
Created attachment 118051 [details] [review] enable bypassing of link attribute mask Will and I discussed this. Fortunately one of us is smart, and we're going with what he says. ;-) 1. We should enable a way to bypass obtaining an attribute mask for links (as opposed to for everything, which is what I did in my proof-of-concept patch). 2. We should bypass obtaining the attribute mask for links in Gecko only. 3. Doing so will cause links to not be underlined in braille in Gecko, but, oh yeah, they're not being underlined as it is. :-) So priority #2 will be to figure out why the Firefox underlining links patch Eitan did is not passing the regression tests and get it working as best as we can ASAP. This patch has been pylinted and manually tested in OOo writer and Firefox. I've not yet regression tested it, but it's a minor change (to the code) and we don't have any regression tests that I'm aware off for the underlining of links anyway. Will please review to make sure I'm not doing something (else) amazingly stupid. ;-) ;-) ;-) Thanks!! (p.s. It puts the link code I had changed back to the way it was before my last check-in.)
(In reply to comment #22) > This patch has been pylinted and manually tested in OOo writer and Firefox. > I've not yet regression tested it, but it's a minor change (to the code) and we > don't have any regression tests that I'm aware off for the underlining of links > anyway. > > Will please review to make sure I'm not doing something (else) amazingly > stupid. ;-) ;-) ;-) Thanks!! Looks fabulous. This also helps give us a way to migrate over to what seems like a better way to handle links, which is to use the new braille.Link class from Eitan's patch, assuming we can figure out why there are regressions associated with that patch. Thanks!
Thanks Will! Patch committed to trunk. Moving this to pending and will see what's going on with Eitan's patch now. When I resume working on bug 535178, we should be able to further improve the issue raised in the opening report (because then we hopefully won't be making a ton of little regions for the same line).