Closed Bug 1343964 Opened 7 years ago Closed 7 years ago

stylo: Figure out a performant solution for base URIs

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: manishearth, Assigned: xidorn)

References

(Blocks 2 open bugs)

Details

Attachments

(8 files)

Bug 1343964 part 1 - Move URLExtraData into its own header.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1343964 part 2 - Move dummy url data to be a static member of URLExtraData.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1343964 part 3 - Have ServoStyleSheet own a URLExtraData.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1343964 part 4 - Have document and svg:use element own URLExtraData.
59 bytes, text/x-review-board-request
bholley
: review+
bzbarsky
: review+
Details
Bug 1343964 part 5 - Factor out common part of nsDOMCSSDeclaration::Parse{,Custom}PropertyValue.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1343964 part 6 - Add a const Rule() method for ServoStyleRule.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1343964 part 7 - Use URLExtraData for declaration modification directly for Servo backend.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1343964 part 8 - Update mochitest expectation.
59 bytes, text/x-review-board-request
Details 
Currently for presentation attributes and CSSOM we convert the base uri to a utf8 string and pass it over to servo. This can be expensive, especially for data: uri documents.

We should be doing something better here.
As just discussed on IRC, this is not just expensive at the point when we make the parse call, but makes this sort of SVG (as a data: document, which is not uncommon on the web, unfortunately):

  <svg>
   <!-- Define a bunch of paint servers -->
   <rect fill="url(#foo)"/>
   <rect fill="url(#bar)"/>
   <rect fill="url(#foo)"/>
   <rect fill="url(#bar)"/>
  </svg>

create lots (4 in this case, but in an actual SVG it can easily be hundreds or thousands) of copies of the entire data: URL, for the lifetime of the doc.  This is terrible.

Gecko handles this by having "#foo" resolved relative to a base data: URI actually share most of the string data, except for the "#foo" bit.  So in the above testcase there would still be only one copy of the entire URL in Gecko.
We also shouldn't be constructing the threadsafeuris each time, according to xidorn.
Assignee: nobody → xidorn+moz
Priority: -- → P1
Xidorn, does it make sense to roll this into bug 1310886, or is are the CSSOM/presentational attributes going to need special handling?
Flags: needinfo?(xidorn+moz)
IIRC manish's patch somewhere is going to fix bug 1310886 with a less performant way, so we can probably leave the bug open here and fix the performance issue in this bug.
Flags: needinfo?(xidorn+moz)
See bug 1343964 comment 2 for xidorn's proposal.
Summary: stylo: Figure out a solution for base URIs → stylo: Figure out a performant solution for base URIs
> See bug 1343964 comment 2 for xidorn's proposal.

That's this bug...
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> See bug 1343964 comment 2 for xidorn's proposal.

I think you mean bug 1310886 comment 2.
Another thing to consider here: we're currently using rust-url for this stuff, which is slow. I'm eliminating rust-url usage for specified values in bug 1347435, and we should potentially do the same here.
Blocks: 1349438
So, my plan for fixing this is:
1. Create a new thread-safe refcounted struct, probably called GeckoURLExtraData, to bundle baseURI, referrer, and principal, and we pass the struct as a whole across FFI boundary, so that we don't need individual thread-safe holders for those three objects.
2. Create a new alias, probably called UrlExtraData, in Servo side, which represents ServoUrl for Servo, and RefPtr<GeckoURLExtraData> for Gecko, to replace ParserContext::{base_url,extra_data}.
3. Make all objects which can own a base url to also own a URLExtraData (document, stylesheet, and svg use element), and then we pass a reference to Servo for parsing.

With the first step, we can reduce the overhead of having too many thread-safe holders (which is not strictly related to this bug, though, so I can probably put it into a separate bug to make landing code easier).

The second step would unify most of URL handling of Servo and Stylo, and leave the only difference to live in {servo,gecko}::url.

The third step would eventually fix this issue, that no allocation/refcounting would be invoked when not necessary.
Depends on: 1351957
Depends on: 1352025
It seems my proposed patchset fixes servo/servo#15950 as expected.
Comment on attachment 8854333 [details]
Bug 1343964 part 1 - Move URLExtraData into its own header.

https://reviewboard.mozilla.org/r/126272/#review129246

::: commit-message-44dd6:3
(Diff revision 1)
> +Bug 1343964 part 1 - Move URLExtraData into its own header. r?heycam
> +
> +This patch does the following in addition to a simply move:

s/simply/simple/

::: layout/style/ServoBindingTypes.h:22
(Diff revision 1)
>  namespace mozilla {
>    class ServoElementSnapshot;
>    struct StyleAnimation;
> -namespace css {
> -struct URLExtraData;
> +  struct URLExtraData;
> -} // namespace css
>  namespace dom {

Nit: I think the prevailing style is not to indent forward declarations inside namespaces like this, so feel free to not indent this one and unindent the ones above it.
Attachment #8854333 - Flags: review?(cam) → review+
Comment on attachment 8854334 [details]
Bug 1343964 part 2 - Move dummy url data to be a static member of URLExtraData.

https://reviewboard.mozilla.org/r/126274/#review129250

::: layout/style/URLExtraData.h:44
(Diff revision 1)
>  
>    nsIURI* BaseURI() const { return mBaseURI; }
>    nsIURI* GetReferrer() const { return mReferrer; }
>    nsIPrincipal* GetPrincipal() const { return mPrincipal; }
>  
> +  static URLExtraData* Dummy() { return sDummy; }

Maybe MOZ_ASSERT in here that it's not null?
Attachment #8854334 - Flags: review?(cam) → review+
Comment on attachment 8854335 [details]
Bug 1343964 part 3 - Have ServoStyleSheet own a URLExtraData.

https://reviewboard.mozilla.org/r/126276/#review129254

::: layout/style/ServoStyleSheet.h:37
(Diff revision 1)
> +  // XXX StyleSheetInfo already has mSheetURI, mBaseURI, and mPrincipal.
> +  // Can we somehow replace them with URLExtraData directly? The issue
> +  // is currently URLExtraData is immutable, but URIs in StyleSheetInfo
> +  // seems to be mutable, so we probably cannot set them altogether.
> +  // Also, this is mostly a duplicate reference of the same url data
> +  // inside RawServoStyleSheet. We may want to just use that instead.
> +  RefPtr<URLExtraData> mURLData;

Does mReferrer serve the same purpose as mSheetURI?  It might do, given the comment above the mSheetURI declaration in nsCSSParser.cpp.  If it does, then maybe we can just replace those three members with a RefPtr<URLExtraData>, and make SetURIs take an URLExtraData* instead of the three separate members.
Attachment #8854335 - Flags: review?(cam) → review+
Comment on attachment 8854335 [details]
Bug 1343964 part 3 - Have ServoStyleSheet own a URLExtraData.

https://reviewboard.mozilla.org/r/126276/#review129254

> Does mReferrer serve the same purpose as mSheetURI?  It might do, given the comment above the mSheetURI declaration in nsCSSParser.cpp.  If it does, then maybe we can just replace those three members with a RefPtr<URLExtraData>, and make SetURIs take an URLExtraData* instead of the three separate members.

Yes, `mReferrer` serves the same purpose as `mSheetURI`. The issue is that we set principal in different place than URIs, and we want to ensure that principal never changes after its first setting, while it seems URIs can be changed afterwards. I didn't figure out a trivial way to replace them. I guess we can have a followup bug filed for this, and investigate that in the future. WDYT?
Comment on attachment 8854337 [details]
Bug 1343964 part 5 - Factor out common part of nsDOMCSSDeclaration::Parse{,Custom}PropertyValue.

https://reviewboard.mozilla.org/r/126280/#review129258

::: layout/style/nsDOMCSSDeclaration.cpp:307
(Diff revision 1)
> -    nsCSSParser cssParser(env.mCSSLoader);
> +    aGeckoFunc(decl->AsGecko(), env, &changed);
> -    cssParser.ParseProperty(aPropID, aPropValue,
> -                            env.mSheetURI, env.mBaseURI, env.mPrincipal,
> -                            decl->AsGecko(), &changed, aIsImportant);
>    } else {
> -    NS_ConvertUTF16toUTF8 value(aPropValue);
> +    changed = aServoFunc(decl->AsServo(), urlData);

Where is urlData defined?
Comment on attachment 8854337 [details]
Bug 1343964 part 5 - Factor out common part of nsDOMCSSDeclaration::Parse{,Custom}PropertyValue.

https://reviewboard.mozilla.org/r/126280/#review129258

> Where is urlData defined?

I see it's defined in a later patch.  So that this patch compiles and still works, can you preserve the RefPtr<URLExtraData> variables, inside the callback functions, and then remove them in that later patch?
Comment on attachment 8854337 [details]
Bug 1343964 part 5 - Factor out common part of nsDOMCSSDeclaration::Parse{,Custom}PropertyValue.

https://reviewboard.mozilla.org/r/126280/#review129258

> I see it's defined in a later patch.  So that this patch compiles and still works, can you preserve the RefPtr<URLExtraData> variables, inside the callback functions, and then remove them in that later patch?

I shouldn't remove
```c++
    // FIXME (bug 1343964): Figure out a better solution for sending the base uri to servo
    RefPtr<URLExtraData> data =
      new URLExtraData(env.mBaseURI, env.mSheetURI, env.mPrincipal);
```
in this commit...

Anyway, it is not needed anymore after part 7. I can add it back for completeness of this patch.
Comment on attachment 8854338 [details]
Bug 1343964 part 6 - Add a const Rule() method for ServoStyleRule.

https://reviewboard.mozilla.org/r/126282/#review129266
Attachment #8854338 - Flags: review?(cam) → review+
Comment on attachment 8854339 [details]
Bug 1343964 part 7 - Use URLExtraData for declaration modification directly for Servo backend.

https://reviewboard.mozilla.org/r/126284/#review129268

::: layout/style/nsDOMCSSDeclaration.h:159
(Diff revision 1)
>    // An implementation for GetCSSParsingEnvironment for callers wrapping
>    // an css::Rule.
>    static void GetCSSParsingEnvironmentForRule(mozilla::css::Rule* aRule,
>                                                CSSParsingEnvironment& aCSSParseEnv);
>  
> +  // An implementation for GetURLData for callers wrapping an css::Rule.

s/an css/a css/

::: layout/style/nsDOMCSSDeclaration.cpp:119
(Diff revision 1)
>    CSSParsingEnvironment env;
> +  URLExtraData* urlData = nullptr;
> +  if (olddecl->IsGecko()) {
> -  GetCSSParsingEnvironment(env);
> +    GetCSSParsingEnvironment(env);
> -  if (!env.mPrincipal) {
> +    if (!env.mPrincipal) {
> -    return NS_ERROR_NOT_AVAILABLE;
> +      return NS_ERROR_NOT_AVAILABLE;
> -  }
> +    }
> +  } else {
> +    urlData = GetURLData();
> +    if (!urlData) {
> +      return NS_ERROR_NOT_AVAILABLE;
> +    }
> +  }

Since CSSParsingEnvironment is used to get the same kind of data as the URLExtraData, it would be nice to have a single mechanism to get the required data to pass along to the parsing function.  Do you think it makes sense to replace the sheet/base/principal points on CSSParsingEnvironment object with a URLExtraData pointer instead?
Comment on attachment 8854339 [details]
Bug 1343964 part 7 - Use URLExtraData for declaration modification directly for Servo backend.

https://reviewboard.mozilla.org/r/126284/#review129268

> Since CSSParsingEnvironment is used to get the same kind of data as the URLExtraData, it would be nice to have a single mechanism to get the required data to pass along to the parsing function.  Do you think it makes sense to replace the sheet/base/principal points on CSSParsingEnvironment object with a URLExtraData pointer instead?

It makes sense but currently `CSSParsingEnvironment` still use `nsCOMPtr` to hold the base url because of `xml:base` support on style attribute, which we haven't completely removed, although it has been disabled by default on Nightly.
Comment on attachment 8854335 [details]
Bug 1343964 part 3 - Have ServoStyleSheet own a URLExtraData.

https://reviewboard.mozilla.org/r/126276/#review129254

> Yes, `mReferrer` serves the same purpose as `mSheetURI`. The issue is that we set principal in different place than URIs, and we want to ensure that principal never changes after its first setting, while it seems URIs can be changed afterwards. I didn't figure out a trivial way to replace them. I guess we can have a followup bug filed for this, and investigate that in the future. WDYT?

Yes, followup sounds good, thanks.
Comment on attachment 8854339 [details]
Bug 1343964 part 7 - Use URLExtraData for declaration modification directly for Servo backend.

https://reviewboard.mozilla.org/r/126284/#review129268

> It makes sense but currently `CSSParsingEnvironment` still use `nsCOMPtr` to hold the base url because of `xml:base` support on style attribute, which we haven't completely removed, although it has been disabled by default on Nightly.

OK.  File a followup that depends on the xml:base removal?
Comment on attachment 8854337 [details]
Bug 1343964 part 5 - Factor out common part of nsDOMCSSDeclaration::Parse{,Custom}PropertyValue.

https://reviewboard.mozilla.org/r/126280/#review129282

r=me with the RefPtr<URLExtraData> stuff restored.
Attachment #8854337 - Flags: review?(cam) → review+
Comment on attachment 8854339 [details]
Bug 1343964 part 7 - Use URLExtraData for declaration modification directly for Servo backend.

https://reviewboard.mozilla.org/r/126284/#review129284
Attachment #8854339 - Flags: review?(cam) → review+
Comment on attachment 8854336 [details]
Bug 1343964 part 4 - Have document and svg:use element own URLExtraData.

https://reviewboard.mozilla.org/r/126278/#review129744

I'm not sure about the svg:use bits, and generally am not super familiar with our URI handling, but given that this blocks your whole stack, I think it's probably ok to have bz do a post-landing sr.

::: dom/base/FragmentOrElement.cpp:449
(Diff revision 1)
> +URLExtraData*
> +nsIContent::GetURLData() const
> +{
> +  if (IsInAnonymousSubtree() && IsAnonymousContentInSVGUseSubtree()) {
> +    nsIContent* bindingParent = GetBindingParent();
> +    MOZ_ASSERT(bindingParent);
> +    SVGUseElement* useElement = static_cast<SVGUseElement*>(bindingParent);
> +    return useElement->GetContentURLData();
> +  }
> +  // This also ignores the case that SVG inside XBL binding.
> +  // But it is probably fine.
> +  return OwnerDoc()->URLData();
> +}

I don't really know enough to review this svg:use business. Would like bz's input here.

::: dom/base/nsDocument.cpp:3586
(Diff revision 1)
> +URLExtraData*
> +nsIDocument::URLData()
> +{
> +#ifdef MOZ_STYLO
> +  nsIURI* baseURI = GetDocBaseURI();
> +  nsIURI* docURI = GetDocumentURI();
> +  nsIPrincipal* principal = NodePrincipal();
> +  if (!mCachedURLData ||
> +      mCachedURLData->BaseURI() != baseURI ||
> +      mCachedURLData->GetReferrer() != docURI ||
> +      mCachedURLData->GetPrincipal() != principal) {
> +    mCachedURLData = new URLExtraData(baseURI, docURI, principal);
> +  }
> +  return mCachedURLData;
> +#else
> +  MOZ_CRASH("Should not be called for non-stylo build");
> +  return nullptr;
> +#endif
> +}

Let's assert that this is main-thread-only.

::: dom/base/nsDocument.cpp:3586
(Diff revision 1)
> +URLExtraData*
> +nsIDocument::URLData()
> +{

Per IRC discussion I think it'd be safer to return already_AddRefed here, but you want to avoid refcounting. I guess I'm ok with it, but it feels a bit sketchy.

::: dom/base/nsIDocument.h:490
(Diff revision 1)
>  
>    virtual void SetBaseURI(nsIURI* aURI) = 0;
>  
>    /**
> +   * Return the URL data which style system needs for resolving url value.
> +   * It would try to use the cached object in mCachedURLData, but if the

s/It would try/This method attempts/

::: dom/base/nsIDocument.h:493
(Diff revision 1)
>    /**
> +   * Return the URL data which style system needs for resolving url value.
> +   * It would try to use the cached object in mCachedURLData, but if the
> +   * base URI or principal has changed since last call to this function,
> +   * or the function is called the first time for the document, a new one
> +   * would be created.

"new one is created".

::: dom/svg/SVGUseElement.cpp:331
(Diff revision 1)
> -  mContentBaseURI = targetContent->GetBaseURI();
> -  if (!mContentBaseURI) {
> +  nsCOMPtr<nsIURI> baseURI = targetContent->GetBaseURI();
> +  if (!baseURI) {
>      return nullptr;
>    }
> +  mContentURLData = new URLExtraData(baseURI.forget(),
> +                                     do_AddRef(GetSourceDocURI()),
> +                                     do_AddRef(NodePrincipal()));

Would like bz's input on this part as well.
Attachment #8854336 - Flags: review+
Blocks: 1353964
Comment on attachment 8854335 [details]
Bug 1343964 part 3 - Have ServoStyleSheet own a URLExtraData.

https://reviewboard.mozilla.org/r/126276/#review129254

> Yes, followup sounds good, thanks.

Filed bug 1353964.
Blocks: 1353967
Comment on attachment 8854339 [details]
Bug 1343964 part 7 - Use URLExtraData for declaration modification directly for Servo backend.

https://reviewboard.mozilla.org/r/126284/#review129268

> OK.  File a followup that depends on the xml:base removal?

Filed bug 1353967.
Comment on attachment 8854340 [details]
Bug 1343964 part 8 - Update mochitest expectation.

https://reviewboard.mozilla.org/r/126286/#review129774
Attachment #8854340 - Flags: review+
There are non-trivial conflicts on binding files at the moment, and m-c is currently broken for Windows stylo build, so I probably have to try landing the patches tomorrow.
Comment on attachment 8854336 [details]
Bug 1343964 part 4 - Have document and svg:use element own URLExtraData.

https://reviewboard.mozilla.org/r/126278/#review129806

r=me with the nits below.

::: dom/base/FragmentOrElement.cpp:458
(Diff revision 2)
> +    nsIContent* bindingParent = GetBindingParent();
> +    MOZ_ASSERT(bindingParent);
> +    SVGUseElement* useElement = static_cast<SVGUseElement*>(bindingParent);
> +    return useElement->GetContentURLData();
> +  }
> +  // This also ignores the case that SVG inside XBL binding.

So this ignores the nsLayoutUtils::StyleAttrWithXMLBaseDisabled() pref.  I guess we assume it'll be true by the time we're shipping stylo?

Might be worth a MOZ_ASSERT to catch that assumption being false.

::: dom/base/nsIContent.h:966
(Diff revision 2)
>  
>    // Returns base URI for style attribute.
>    already_AddRefed<nsIURI> GetBaseURIForStyleAttr() const;
>  
> +  // Returns the URL data for style attribute.
> +  mozilla::URLExtraData* GetURLData() const;

If it's style attr specific, please reflect that in the name.

::: dom/base/nsIDocument.h:491
(Diff revision 2)
>    virtual void SetBaseURI(nsIURI* aURI) = 0;
>  
>    /**
> +   * Return the URL data which style system needs for resolving url value.
> +   * This method attempts to use the cached object in mCachedURLData, but
> +   * if the base URI or principal has changed since last call to this

Or the document uri, right?  That can actually change (unlike the principal, iirc).

::: dom/base/nsIDocument.h:495
(Diff revision 2)
> +   * This method attempts to use the cached object in mCachedURLData, but
> +   * if the base URI or principal has changed since last call to this
> +   * function, or the function is called the first time for the document,
> +   * a new one is created.
> +   */
> +  mozilla::URLExtraData* URLData();

Again, this is a very generic name for a thing with _very_ specific semantics.  Please rename to something that reflects those semantics.  Maybe DefaultStyleAttrURLData()?

::: dom/svg/SVGUseElement.cpp:336
(Diff revision 2)
> -  mContentBaseURI = targetContent->GetBaseURI();
> -  if (!mContentBaseURI) {
> +  nsCOMPtr<nsIURI> baseURI = targetContent->GetBaseURI();
> +  if (!baseURI) {
>      return nullptr;
>    }
> +  mContentURLData = new URLExtraData(baseURI.forget(),
> +                                     do_AddRef(GetSourceDocURI()),

This changes the referrer for inline style on <use>-imported nodes, right?

That might be OK, but we could also just preserve our old behavior for now, and make the behavior change in a separate bug.  I would slightly prefer that.
Attachment #8854336 - Flags: review+
Xidorn, do your patches address the problem comment 1 describes, or is a new bug needed on that?
Flags: needinfo?(xidorn+moz)
As discussed in IRC, comment 1 should never really an issue, because we always delegate relative URI resolution to Gecko code since it's initial implementation in bug 1288302. We were unnecessarily serializing URLs when we call into Servo functions, but that issue should have been fixed by bug 1352025 now.
Flags: needinfo?(xidorn+moz)
Servo side change: servo/servo#16282
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/79fb83faeb2a
part 1 - Move URLExtraData into its own header. r=heycam
https://hg.mozilla.org/integration/autoland/rev/7ee162581b5c
part 2 - Move dummy url data to be a static member of URLExtraData. r=heycam
https://hg.mozilla.org/integration/autoland/rev/cb21e116df50
part 3 - Have ServoStyleSheet own a URLExtraData. r=heycam
https://hg.mozilla.org/integration/autoland/rev/a37bb4a37b01
part 4 - Have document and svg:use element own URLExtraData. r=bholley,bz
https://hg.mozilla.org/integration/autoland/rev/78aed6715e29
part 5 - Factor out common part of nsDOMCSSDeclaration::Parse{,Custom}PropertyValue. r=heycam
https://hg.mozilla.org/integration/autoland/rev/6caf71be3010
part 6 - Add a const Rule() method for ServoStyleRule. r=heycam
https://hg.mozilla.org/integration/autoland/rev/905554c0bc81
part 7 - Use URLExtraData for declaration modification directly for Servo backend. r=heycam
https://hg.mozilla.org/integration/autoland/rev/99572645ff7f
part 8 - Update mochitest expectation. r=xidorn
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9c639ec492e5
followup - Add back code change which was incorrectly dropped when addressing review comment on a CLOSED TREE.
Depends on: 1354772
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: