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.

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().

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

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.... ;)

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

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

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.

>> 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.

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.