Warren Harris Assignee
	Description • 25 years ago

The issue here is that MakeAbsolute is a bottleneck when displaying pages
because it's called to resolve whether the page has been visited previously.

Although we should look at speeding up MakeAbsolute, perhaps this link coloring
feature should be done asynchronously to page loading. I'm sure that's a big
change, and I'm not sure who would own it.

	leger
	Comment 1 • 25 years ago

Putting on [Perf] radar.

	Warren Harris Assignee
	Comment 2 • 25 years ago

*** Bug 2029 has been marked as a duplicate of this bug. ***

	Warren Harris Assignee
	Comment 3 • 25 years ago

So I heard today that MakeAbsolute performance is a major problem for the style
resolution system, not just mousing over links as I previously though. Can
someone that I've cc'd on this please fill us in with details, and what to
do/load in order to see it show up in a quantify run? Thanks.

	chris hofmann Reporter
	Comment 4 • 25 years ago

hyatt/waterson,  did you have poop on this?

	Peter Linss
	Comment 5 • 25 years ago

Simply load a page that has a large number of links on it. The sample URL
http://www.w3.org/TR/REC-CSS2/ is a good one.
For each link we have to compute the absolute URL in order to test if the link
has been visited or not. This gets done once for each style rule that cares
about link state per link in the page.

	Warren Harris Assignee
	Comment 6 • 25 years ago

See Simon's related bug #11677 for more clues.

	rbs
	Comment 7 • 25 years ago

Warren's suggestion to do link coloring asynchronously sounds promising
to me. Since it would be very tricky to implement, what about trying a
simpler alternative consisting of the following passes:
- Add another frame reflow status (e.g., FRAME_NEEDS_VISUAL_REFLOW or whatever!)
- Lay the document first. At this stage, forgetting about resolving the
coloring style of anchor frames, and marking them as FRAME_NEEDS_VISUAL_REFLOW.
- Then upon laying the document, fire an extra reflow (with the VISUAL hint)
targetted at anchor frames. When they receive this reflow command, they can
then resolve their color and complete their reflow status.

Because the coloring doesn't in fact affect the formatting, it can be treated
*after* the document is displayed, using the (visual) restyle logic built in
the style stystem. While it is not a parallel separation (as Warren suggested),
doing this sequential separation can nevertheless improve the rendering of
large documents (see also bug 11677).

Hope it makes sense.
Of course, MakeAbsolute still needs an improvement and if it ends up fast
enough, all this separation would not be necessary...

	Warren Harris Assignee
	Comment 8 • 25 years ago

While you're at it, you (or someone) should change one thing that's always
bugged me about link coloring -- the fact that you sometimes have to reload the
page to get the link colors to change. This can happen when you right-click on a
link and do an 'open in new window' or something like that. The link should
change color right then -- not after the next reload. (Let me know if this
should be a separate bug report.)

	Warren Harris Assignee
	Comment 9 • 25 years ago

Also, the work that rbs@maths.uq.edu.au described probably can't/shouldn't be
handled by Gagan. Maybe peterl can comment on who should own it.

	Peter Linss
	Comment 10 • 25 years ago

Lets fix MakeAbsolute before we go off an invent a whole new system that we may
not need. Handling link style in an async manner is not as simple as
rbs@maths.uq.edu.au describes. It also has already been discussed and is a known
path available if we still have performance problems *after* we fix this.

The improved link colloring (for open in new window/link in same doc) issue is
already covered under another bug.

	rbs
	Comment 11 • 25 years ago

Doing the coloring in async mode is obviously not trivial, so I was instead
betting on something sequential, i.e., another pass. But if MakeAbsolute
is fast enough, it is not necessary to do extra things. So it is better
to concentrate on tuning MakeAbsolute first, and only look elsewhere as
last resort.

	Gagan
	Comment 12 • 25 years ago

Jud: once you are done checking in your changes, hand this bug back to me. I
have some more optimizations sitting my tree that would be related to this.

	Judson Valeski
	Comment 13 • 25 years ago

I have a fix from andreas for this.

	chris hofmann Reporter
	Comment 14 • 25 years ago

how risky is the fix?  how much testing has been done?
is this something we might want to think about getting into m11?

	Judson Valeski
	Comment 15 • 25 years ago

fix checked in 11/04/99 12:35pm pac time, handing back to gagan per his request.

	leger
	Comment 16 • 25 years ago

Bulk move of all Necko (to be deleted component) bugs to new Networking

component.

	Warren Harris Assignee
	Comment 17 • 25 years ago

This was fixed a month or so ago.

	Paul MacQuiddy
	Comment 18 • 25 years ago

trusting warren

	Warren Harris Assignee
	Comment 19 • 25 years ago

Reopening with more perf data from Troy:

Troy Chevalier wrote:

  Warren,

  Here are things I noticed looking at the Quantify data for a load of 
http://www.amazon.com/

  - 1,344 calls to NS_MakeAbsoluteURI() (mostly called from the style system) 
ended up calling ns StdURL() 4,189 times 
  which resulted in 16,314 calls to nsStr::Alloc(). That's a lot of calls to 
malloc()

Assigning to myself since I have some fixes to improve this.

	Warren Harris Assignee
	Comment 20 • 25 years ago

=> me

	Andreas Otte
	Comment 21 • 25 years ago

The malloc count certainly rose with the latest checkin to the urlparser because
we now do the escaping. That comes with a price.

	Warren Harris Assignee
	Comment 22 • 25 years ago

My fix is to optimize the number of calls to GetService -- I was seeing that 
show up a lot. The malloc thing should be looked at too though -- maybe do more 
in nsAutoString buffers, and less copying -- but I haven't studied the code 
yet.

	Andreas Otte
	Comment 24 • 25 years ago

Adding Scott to the CC list

	Brendan Eich [:brendan]
	Comment 25 • 25 years ago

Shouldn't esc_str in 

nsIOService::Escape(const char *str, PRInt16 mask, char** result)
{
    nsCString esc_str;

be an nsCAutoString?

/be

	Andreas Otte
	Comment 26 • 25 years ago

Of course ... should be ... thanks

	troy
	Comment 27 • 25 years ago

Nominating we fix it for beta1

	leger
	Comment 28 • 25 years ago

Do you have a fix in hand at this point?

	troy
	Comment 29 • 25 years ago

A fix has been discussed and Scott Putterman proposed some changes. Warren and 
Andreas will know for sure

	leger
	Comment 30 • 25 years ago

Putting on PDT+ radar for beta1.

	Warren Harris Assignee
	Comment 32 • 25 years ago

Fix checked in.

Running quantify, counting the time for complete startup and visiting 3 pages, 
I saw MakeAbsolute take 2.33% of total time before and 1.6% after this patch 
was applied. Note that this will be a bigger savings when startup time is 
factored out.

	Warren Harris Assignee
	Comment 33 • 25 years ago

fixed

	troy
	Comment 34 • 25 years ago

Did you go to www.amazon.com and look and see how many strings were allocated 
because of calls to NS_MakeAbsoluteURI()? The number of string temporarily 
allocated/freed was the major problem I noticed.

	scottputterman
	Comment 35 • 25 years ago

I have what I think is another improvement.  Should I reopen this or start a new 
bug>  Anyway, I will keep writing in here.  I opened the browser, chose my 
profile, viewed mozilla.org, yahoo, amazon, a page on amazon, and Tinderbox in 
both runs.

In the first run nsString::ToNewUTF8 string was called 13,397 times for 232,442 
microseconds.  In the second run, the function was called 12,513 times for 
102,227 microseconds.  About 50,000 of the saved time was in MakeAbsolute which 
cut it down by an 1/8 (old time a little above 400,000 and the new was about 
350,000).

I'll attach the patch later.  I think we unnecessarily copied the original 
string into a temporary string given that we are just going to copy it again 
using UTF8.  I think this is working the same as before.

	Warren Harris Assignee
	Comment 37 • 25 years ago

Scott: Can you get rickg to review this one?

	Paul MacQuiddy
	Comment 38 • 25 years ago

verified, page loading on this page is on the same order as 4.x