Closed
Bug 1160628
Opened 9 years ago
Closed 9 years ago
Speed up the DOM URL constructor
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(2 files, 1 obsolete file)
16.10 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
13.27 KB,
patch
|
Details | Diff | Splinter Review |
It has some issues. In particular: 1) Slow getting of the IO service. 2) Slow construction of about:blank URIs (this is _really_ slow). The upshot is that this testcase: var count = 150000; var start = new Date; for (var i = 0; i < count; ++i) var url = new URL("http://web.mit.edu") alert((new Date - start) / count * 1e6); becomes about 5-6x faster.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8600441 -
Flags: review?(amarchesini)
Comment 2•9 years ago
|
||
Comment on attachment 8600441 [details] [diff] [review] Speed up the URL constructor in the common case of no base URI Review of attachment 8600441 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/URL.cpp @@ +75,5 @@ > + ErrorResult& aRv) > +{ > + if (aBase.EqualsLiteral("about:blank") && !sHaveShutDown) { > + if (!sAboutBlankURI) { > + nsresult rv = NS_NewURI(&sAboutBlankURI, "about:blank"); aRv = NS_NewURI(...) if (NS_WARN_IF(aRv.Failed()) { NS_IF_RELEASE(sAboutBlankURI); return nullptr; } Any reason why use a separate nsresult? @@ +93,1 @@ > if (NS_FAILED(rv)) { Can you add NS_WARN_IF? @@ +108,1 @@ > if (NS_FAILED(rv)) { same here. ::: dom/base/URL.h @@ +129,5 @@ > + // Versions of Constructor that we can share with workers. > + static already_AddRefed<URL> > + Constructor(const nsAString& aUrl, const nsAString& aBase, ErrorResult& aRv); > + static already_AddRefed<URL> > + Constructor(const nsAString& aUrl, nsIURI* aBase, ErrorResult& aRv); Can you move those where the other Constructor methods are?
Attachment #8600441 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 3•9 years ago
|
||
Per IRC discussion, going to update this to the spec change that now the default is "no base URI" instead of about:blank....
Assignee | ||
Comment 4•9 years ago
|
||
The spec just got changed to make the second arg optional, with no base URL used when not passed, so we no longer need the about:blank cache
Attachment #8601528 -
Flags: review?(amarchesini)
Assignee | ||
Updated•9 years ago
|
Attachment #8600441 -
Attachment is obsolete: true
Assignee | ||
Comment 5•9 years ago
|
||
Comment 6•9 years ago
|
||
Comment on attachment 8601528 [details] [diff] [review] Updated to spec changes Review of attachment 8601528 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/URL.cpp @@ +269,5 @@ > > + nsRefPtr<mozilla::dom::URL> url; > + if (mBaseProxy) { > + url = mozilla::dom::URL::Constructor(mURL, mBaseProxy->URI(), mRv); > + } else if (mHaveBaseString) { can you use mBase.IsVoid() instead having a separate boolean?
Attachment #8601528 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 7•9 years ago
|
||
I could, I guess. I sort of try to avoid using void strings, because we keep talking about removing them. But I guess this use case would be easy to fix up at that point.
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/09660c20ac70
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•