If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

mozilla very slow to render page

RESOLVED FIXED

Status

()

Core
Layout: Text
RESOLVED FIXED
14 years ago
9 years ago

People

(Reporter: H. Ebelt, Assigned: smontagu)

Tracking

({perf})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

14 years ago
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)
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.
Assignee: general → mkaply
Status: UNCONFIRMED → NEW
Component: Browser-General → BiDi Hebrew & Arabic
Ever confirmed: true
Keywords: perf
OS: Windows XP → All
QA Contact: general → zach
Hardware: PC → All
Summary: mozilla hangs → mozilla very slow to render page
(Assignee)

Comment 2

14 years ago
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().
Assignee: mkaply → shafalus
(Assignee)

Updated

14 years ago
Blocks: 188294
(Assignee)

Comment 3

14 years ago
Created attachment 128181 [details] [diff] [review]
Patch as described in previous comment

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

Updated

14 years ago
Blocks: 182985
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.... ;)

Updated

14 years ago
Blocks: 140841
(Assignee)

Comment 5

14 years ago
Created attachment 128275 [details] [diff] [review]
Patch v.2

This incorporates corrections of nsDataHashtable usage based on bsmedberg's
comments on IRC, and adds some code commentary.
Attachment #128181 - Attachment is obsolete: true
(Assignee)

Comment 6

14 years ago
Created attachment 128294 [details] [diff] [review]
Some more cleanup

Much the same, but with some more code shuffled in and out of
RemoveBidiContinuation().
Attachment #128275 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #128294 - Flags: superreview?(bzbarsky)
Attachment #128294 - Flags: review?(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.
Attachment #128294 - Flags: superreview?(bzbarsky)
Attachment #128294 - Flags: superreview+
Attachment #128294 - Flags: review?(bzbarsky)
Attachment #128294 - Flags: review+
(Assignee)

Comment 8

14 years ago
>>   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.
(Assignee)

Comment 9

14 years ago
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?
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.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

9 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.