Closed
Bug 10736
Opened 26 years ago
Closed 25 years ago
sched - MakeAbsolute performance (supporting large files) (gagan, )
Categories
(Core :: Networking, defect, P3)
Tracking
()
VERIFIED
FIXED
M14
People
(Reporter: chofmann, Assigned: warrensomebody)
References
()
Details
(Keywords: perf, Whiteboard: [PDT+])
Attachments
(3 files)
10.65 KB,
patch
|
Details | Diff | Splinter Review | |
10.39 KB,
patch
|
Details | Diff | Splinter Review | |
479 bytes,
patch
|
Details | Diff | Splinter Review |
Reporter | ||
Updated•26 years ago
|
Blocks: 10730
Summary: sched - MakeAbsolute performance (supporting large files) (gagan, ) → sched - MakeAbsolute performance (supporting large files) (gagan, )
Assignee | ||
Updated•26 years ago
|
Target Milestone: M11
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.
Assignee | ||
Updated•25 years ago
|
Severity: normal → major
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.
Updated•25 years ago
|
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'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...
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.)
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.
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.
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.
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.
Updated•25 years ago
|
Status: NEW → ASSIGNED
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?
Updated•25 years ago
|
Assignee: valeski → gagan
Status: ASSIGNED → NEW
Comment 15•25 years ago
|
||
fix checked in 11/04/99 12:35pm pac time, handing back to gagan per his request.
Comment 16•25 years ago
|
||
Bulk move of all Necko (to be deleted component) bugs to new Networking component.
Assignee | ||
Updated•25 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
Updated•25 years ago
|
Status: RESOLVED → VERIFIED
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Target Milestone: M13 → M15
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.
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.
Comment 23•25 years ago
|
||
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
Comment 29•25 years ago
|
||
A fix has been discussed and Scott Putterman proposed some changes. Warren and Andreas will know for sure
Assignee | ||
Updated•25 years ago
|
Target Milestone: M15 → M14
Comment 31•25 years ago
|
||
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.
Assignee | ||
Comment 33•25 years ago
|
||
fixed
Status: NEW → RESOLVED
Closed: 25 years ago → 25 years ago
Resolution: --- → FIXED
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.
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.
Comment 36•25 years ago
|
||
Comment 38•25 years ago
|
||
verified, page loading on this page is on the same order as 4.x
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•