Last Comment Bug 597291 - Lazily resolve URLs and other CSS values
: Lazily resolve URLs and other CSS values
Status: VERIFIED FIXED
[ts]
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: mozilla6
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks: abp
  Show dependency treegraph
 
Reported: 2010-09-16 18:19 PDT by (dormant account)
Modified: 2011-07-20 05:35 PDT (History)
8 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
zip file with two testcases testing lots of url rules (one about:, one chrome:) (18.84 KB, application/zip)
2011-04-14 15:10 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details
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 (14.53 KB, patch)
2011-04-14 15:14 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
dbaron: review+
Details | Diff | Review

Description (dormant account) 2010-09-16 18:19:54 PDT
This should be an easy win due to how slow our url code is.
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-09-17 06:32:41 PDT
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.
Comment 2 (dormant account) 2011-04-11 16:30:32 PDT
(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
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-11 16:47:41 PDT
I'd say add a boolean to nsCSSValue::URL.  We can probably also use it to simplify the image-loading stuff.
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-14 15:10:31 PDT
Created attachment 526116 [details]
zip file with two testcases testing lots of url rules (one about:, one chrome:)
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-14 15:14:02 PDT
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
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-14 15:19:57 PDT
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.
Comment 7 (dormant account) 2011-04-14 15:40:31 PDT
(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?
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-14 15:58:35 PDT
What was the end of comment 5 supposed to say?
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-14 17:32:01 PDT
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.  :(
Comment 10 Wladimir Palant 2011-04-14 23:00:58 PDT
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.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-11 01:20:08 PDT
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
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-11 19:02:25 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/d5728312d756
Comment 13 Simona B [:simonab] 2011-07-20 05:35:20 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.