Closed Bug 1160628 Opened 4 years ago Closed 4 years ago

Speed up the DOM URL constructor

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
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+
Per IRC discussion, going to update this to the spec change that now the default is "no base URI" instead of about:blank....
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)
Attachment #8600441 - Attachment is obsolete: true
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+
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.
https://hg.mozilla.org/mozilla-central/rev/09660c20ac70
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1262948
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.