212827 - mozilla very slow to render page

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[H. Ebelt]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de-AT; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-AT; rv:1.4) Gecko/20030624

https://pause.perl.org/pause/query?ACTION=who_is
http://pause.perl.org/pause/query?ACTION=who_is

browser hangs while loading the page (animated icon stops animating, screen is
blank and no return message to windows).




Reproducible: Always

Steps to Reproduce:
1. Load this url

Actual Results:  
hangs up

Expected Results:  
show the page (render it)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Renders the page fine here, after thinking about it for a while (order of a
minute or so).  A profile shows:

Total hit count: 831
Count %Total  Function Name
428   51.5    
_ZNK15nsBidiPresUtils22RemoveBidiContinuationEP14nsIPresContextP8nsIFrameS3_P10nsIContentRiS6_

and indeed, there is a single name in hebrew way down at the bottom of the page.

If we can reduce the time spent in that function, profiling the rest of the
pageload may make sense.

Comment 2

[Simon Montagu :smontagu]

I have a patch for this which is not fully worked out yet: I will attach it as
soon as I am on a system with a working build environment and CVS.

The basic idea is that while building the array of frames in
nsBidiPresUtils::CreateLogicalBuffer() we should create a
nsDataHashtable<nsIContent*, PRUint32> where the PRUint32 corresponds to the
index of mLogicalBuffer and the nsIContent* is the content of each frame in the
buffer. Then when mLogicalBuffer is complete, the hashtable will point to the
last frame in the buffer for each nsIContent*, and we can use it instead of the
current loop at the beginning of nsBidiPresUtils::RemoveBidiContinuation().

Comment 3

[Simon Montagu :smontagu]

Attachment: Patch as described in previous comment

It would be nice if someone could re-profile with this patch.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Only listing functions that took at least 0.5% of total time, without that patch:

Total hit count: 668
Count %Total  Function Name
428   64.1    
_ZNK15nsBidiPresUtils22RemoveBidiContinuationEP14nsIPresContextP8nsIFrameS3_P10nsIContentRiS6_
 13   1.9     _end
  9   1.3     _Z18GetMapFor10646FontP11XFontStruct
  4   0.6     _ZNK15nsBidiPresUtils17CalculateCharTypeERiiS0_S0_S0_RhS1_
  4   0.6     _ZN14nsStyleContext12GetStyleDataE15nsStyleStructID
  4   0.6     _ZN13nsCOMPtr_base16begin_assignmentEv
  4   0.6     _ZN13nsCOMPtr_baseD2Ev

with the patch (only listing things that took over 1% of the time now, since the
time is about 3 times smaller):

Total hit count: 224
Count %Total  Function Name
 17   7.6     _end
  9   4.0     _Z18GetMapFor10646FontP11XFontStruct
  4   1.8     _ZN7CNavDTD16HandleStartTokenEP6CToken
  4   1.8     _ZN13nsCOMPtr_baseD2Ev
  4   1.8     SearchTable
  4   1.8     __i686.get_pc_thunk.bx
  3   1.3     .L122
  3   1.3    
_ZNK8nsString19GetReadableFragmentER18nsReadableFragmentItE17nsFragmentRequestj
  3   1.3     _ZN14nsStyleContext12GetStyleDataE15nsStyleStructID

I still see a bit of a freeze-up on initial pageload, but this is a good first
step.... ;)

Comment 5

[Simon Montagu :smontagu]

Attachment: Patch v.2

This incorporates corrections of nsDataHashtable usage based on bsmedberg's
comments on IRC, and adds some code commentary.

Comment 6

[Simon Montagu :smontagu]

Attachment: Some more cleanup

Much the same, but with some more code shuffled in and out of
RemoveBidiContinuation().

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 128294 [details] [diff] [review]
Some more cleanup

>Index: mozilla/layout/base/public/nsBidiPresUtils.h
>+  void RemoveBidiContinuation(nsIPresContext* aPresContext,
>+                              nsIFrame*       aFrame,
>+                              PRInt32         aFirstIndex,
>+                              PRInt32         aLastIndex,
>+                              PRInt32         aOffset) const;

Hey, toss in a comment about what the indices and offset are?

>Index: mozilla/layout/base/src/nsBidiPresUtils.cpp
>   aForceReflow = PR_FALSE;
>   mLogicalFrames.Clear();
>+  mContentToFrameIndex.Clear();

What happened to clearing these at the exit of the function like we talked
about?	Are there too many return points?

>+  nsCOMPtr<nsIFrameManager> frameManager;
>+  presShell->GetFrameManager(getter_AddRefs(frameManager) );

nsIFrameManager* frameManager = presShell->GetFrameManager();

With those nits picked and the Clear() thing clarified, looks great.  r+sr=me.

Comment 8

[Simon Montagu :smontagu]

>>   mLogicalFrames.Clear();
>>+  mContentToFrameIndex.Clear();
>
>What happened to clearing these at the exit of the function like we talked
>about?	Are there too many return points?

I think there are, and I want to add even more after you pointed out on IRC that
we don't check for success when appending to the arrays.

I've picked your other nits.

Comment 9

[Simon Montagu :smontagu]

Fix checked in. bz, do you want to keep this open for the freeze-up you mention
in comment 4, or will you open a new bug for that?

Comment 10

[Boris Zbarsky [:bzbarsky]]

That freeze-up looks like font lookups to me... We really need to make that code
be faster somehow...

So marking this fixed.  I'll reprofile the dependencies.