Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Lazily resolve URLs and other CSS values

VERIFIED FIXED in mozilla6

Status

()

Core
CSS Parsing and Computation
P1
normal
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: (dormant account), Assigned: bz)

Tracking

(Blocks: 1 bug)

unspecified
mozilla6
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ts])

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
This should be an easy win due to how slow our url code is.
(Assignee)

Comment 1

7 years ago
So we can either add a boolean flag to nsCSSValue::URL to indicate whether it's been resolved or add a new unit to nsCSSValue.  The latter doesn't add a word of bloat to URL, but probably requires more callsites to be updated.  David, preference?

We also need to add a base URI to nsCSSValue::URL.  We can probably reuse the existing URI field for this, so this won't increase the size.
(Reporter)

Comment 2

6 years ago
(In reply to comment #1)
> So we can either add a boolean flag to nsCSSValue::URL to indicate whether it's
> been resolved or add a new unit to nsCSSValue.  The latter doesn't add a word
> of bloat to URL, but probably requires more callsites to be updated.  David,
> preference?

dbaron, waiting on your input on above question
Assignee: nobody → bzbarsky
I'd say add a boolean to nsCSSValue::URL.  We can probably also use it to simplify the image-loading stuff.
(Assignee)

Comment 4

6 years ago
Created attachment 526116 [details]
zip file with two testcases testing lots of url rules (one about:, one chrome:)
(Assignee)

Comment 5

6 years ago
Created attachment 526117 [details] [diff] [review]
Create nsIURI objects lazily for nsCSSValue::URL, so that we don't pay the cost of creating the ones we don't actually need.   the new setup, the mURL member of nsCSSValue::URL stores either the actual URI pointed to or the base URI; a boolean flag keeps
Attachment #526117 - Flags: review?(dbaron)
(Assignee)

Comment 6

6 years ago
On the attached testcases, an opt Mac build on my hardware without the patch shows times of about 470ms for bar.html and 370ms for baz.html.

With the patch, those become 150ms and 170ms respectively.
(Reporter)

Comment 7

6 years ago
(In reply to comment #6)
> On the attached testcases, an opt Mac build on my hardware without the patch
> shows times of about 470ms for bar.html and 370ms for baz.html.
> 
> With the patch, those become 150ms and 170ms respectively.

win! Is there a css benchmark we can add a the lots of urls testcase to?
What was the end of comment 5 supposed to say?
(Assignee)

Comment 9

6 years ago
Comment 5 was auto-generated by bzexport; it's just the checkin comment in the patch.

Taras, we don't have a useful css benchmark.  :(
(Assignee)

Updated

6 years ago
Whiteboard: [ts] → [need review][ts]

Comment 10

6 years ago
Tested Boris' try build - normally the nsIStyleSheetService.loadAndRegisterSheet() call in Adblock Plus takes 260-300ms for me (almost 1 MB of CSS), with this patch it is down to 70ms.
Blocks: 467520
(Assignee)

Updated

6 years ago
Priority: -- → P1
Comment on attachment 526117 [details] [diff] [review]
Create nsIURI objects lazily for nsCSSValue::URL, so that we don't pay the cost of creating the ones we don't actually need.   the new setup, the mURL member of nsCSSValue::URL stores either the actual URI pointed to or the base URI; a boolean flag keeps

r=dbaron
Attachment #526117 - Attachment description: Create nsIURI objects lazily for nsCSSValue::URL, so that we don't pay the cost of creating the ones we don't actually need. the new setup, the mURL member of nsCSSValue::URL stores either the actual URI pointed to or the base URI; a boolean flag keeps → Create nsIURI objects lazily for nsCSSValue::URL, so that we don't pay the cost of creating the ones we don't actually need. the new setup, the mURL member of nsCSSValue::URL stores either the actual URI pointed to or the base URI; a boolean flag keeps
Attachment #526117 - Flags: review?(dbaron) → review+
(Assignee)

Updated

6 years ago
Whiteboard: [need review][ts] → [need landing][ts]
(Assignee)

Comment 12

6 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/d5728312d756
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
OS: Linux → All
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: [need landing][ts] → [ts]
Target Milestone: --- → mozilla6
Mozilla/5.0 (Windows NT 5.1; rv:6.0) Gecko/20100101 Firefox/6.0

Verified issue using the attached test cases added in Comment 4. Tested on Win XP, Win 7, Mac OS X 10.6 and Ubuntu 10.10

The time shown on FF 6.0b2 is:

- for Win XP: 138ms for bar.html and 179ms for baz.html (On Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110511 Firefox/6.0a1, time was: 583ms for bar.html and 404ms on baz.html)

- for Win 7: 221ms for bar.html and 287ms for baz.html (On Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110510 Firefox/6.0a1, time was 1069ms for bar.html and 779ms for baz.html.

- for Mac OS X 10.6: 224ms for bar.html and 259ms for baz.html (on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110510 Firefox/6.0a1, time was 850ms on bar.html and 622ms for baz.html

- for Ubuntu 10.10: 235ms for bar.html and 279ms for baz.html (on Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110510 Firefox/6.0a1, time was 1038ms for bar.html and 772ms for baz.html.

Setting status of this issue to VERIFIED FIXED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.