1160628 - Speed up the DOM URL constructor

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Speed up the URL constructor in the common case of no base URI

Comment 2

[Andrea Marchesini [:baku]]

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?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Per IRC discussion, going to update this to the spec change that now the default is "no base URI" instead of about:blank....

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to spec changes

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff from previous patch, for the record

Comment 6

[Andrea Marchesini [:baku]]

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?

Comment 7

[Boris Zbarsky [:bzbarsky]]

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 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09660c20ac70

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09660c20ac70