Cameron McCormack (:heycam) Assignee
	Description • 8 years ago

I have some local patches that make background-image: url() values work, though I still need to work out a proper way to kick off image loading from Servo without appending each one to a RwLock<Vec<...>> (which is what my hacky patches are doing, and which ruins restyle performance).

	Manish Goregaokar [:manishearth]
	Comment 9 • 8 years ago
mozreview-review

Comment on attachment 8800090 [details]
Support url() values in background-image and mask-image in stylo.

https://reviewboard.mozilla.org/r/85116/#review83676

	Xidorn Quan [:xidorn] UTC+11
	Comment 10 • 8 years ago
mozreview-review

Comment on attachment 8800083 [details]
Bug 1288302 - Part 1: Make css::ImageValue constructable OMT.

https://reviewboard.mozilla.org/r/85102/#review83678

::: layout/style/nsCSSValue.cpp:2849
(Diff revision 1)
>                   aString,
>                   do_AddRef(new PtrHolder<nsIURI>(aBaseURI, false)),
>                   do_AddRef(new PtrHolder<nsIURI>(aReferrer)),
>                   do_AddRef(new PtrHolder<nsIPrincipal>(aOriginPrincipal)))
>  {
>    MOZ_ASSERT(NS_IsMainThread());

I think you can remove the main thread check here as far as Initialize() checks that.

	Xidorn Quan [:xidorn] UTC+11
	Comment 11 • 8 years ago
mozreview-review

Comment on attachment 8800084 [details]
Bug 1288302 - Part 2: Pass ImageTracker to style struct image tracking methods instead of nsPresContext.

https://reviewboard.mozilla.org/r/85104/#review83686

::: layout/style/nsStyleStruct.cpp
(Diff revision 1)
>    MOZ_ASSERT(mType == eStyleImageType_Image,
>               "Can't track image when there isn't one!");
>  
> -  // Register the image with the document
> +  aImageTracker->Add(mImage);
> -  nsIDocument* doc = aContext->Document();
> -  if (doc) {

I suppose callers may need null check given this... It seems from code, aContext->Document() is not promised to always return non-null, although I'm not sure when that could happen.

Probably consider adding another overload which accepts nsPresContext* and do the null check there. If you believe Document() would never return null in this case, feel free to re-request review with some explanation.

	Cameron McCormack (:heycam) Assignee
	Comment 12 • 8 years ago
mozreview-review-reply

Comment on attachment 8800084 [details]
Bug 1288302 - Part 2: Pass ImageTracker to style struct image tracking methods instead of nsPresContext.

https://reviewboard.mozilla.org/r/85104/#review83686

> I suppose callers may need null check given this... It seems from code, aContext->Document() is not promised to always return non-null, although I'm not sure when that could happen.
> 
> Probably consider adding another overload which accepts nsPresContext* and do the null check there. If you believe Document() would never return null in this case, feel free to re-request review with some explanation.

I believe aContext->Document() will only return null after nsPresContext::DetachShell() is called.  That DetachShell() call is done at the end of PresShell::Destroy(), and this happens after both the frame tree (which is holding on to some style contexts) is destroyed, and the style set is destroyed (which, through the rule tree, holds on to the remaining style contexts).  So I believe at this point we won't have any more nsStyleFoo::Destroy() calls.  If some style structs were held alive after the pres shell went away, then I think the pres arena would assert that some of its objects weren't destroyed.

	Xidorn Quan [:xidorn] UTC+11
	Comment 13 • 8 years ago
mozreview-review-reply

Comment on attachment 8800084 [details]
Bug 1288302 - Part 2: Pass ImageTracker to style struct image tracking methods instead of nsPresContext.

https://reviewboard.mozilla.org/r/85104/#review83686

> I believe aContext->Document() will only return null after nsPresContext::DetachShell() is called.  That DetachShell() call is done at the end of PresShell::Destroy(), and this happens after both the frame tree (which is holding on to some style contexts) is destroyed, and the style set is destroyed (which, through the rule tree, holds on to the remaining style contexts).  So I believe at this point we won't have any more nsStyleFoo::Destroy() calls.  If some style structs were held alive after the pres shell went away, then I think the pres arena would assert that some of its objects weren't destroyed.

OK. It seems nsPresContext::mDocument is actually never cleared even after it is detached from PresShell. And there is an assertion that when it attaches to shell, there is a document, so not having a document indicates that the nsPresContext is never attached to any PresShell. Given that style structs and frame tree are ultimately owned by PresShell, such a nsPresContext should never be used in this case.

	Xidorn Quan [:xidorn] UTC+11
	Comment 14 • 8 years ago
mozreview-review

Comment on attachment 8800084 [details]
Bug 1288302 - Part 2: Pass ImageTracker to style struct image tracking methods instead of nsPresContext.

https://reviewboard.mozilla.org/r/85104/#review83756

	Xidorn Quan [:xidorn] UTC+11
	Comment 15 • 8 years ago
mozreview-review

Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.

https://reviewboard.mozilla.org/r/85106/#review83958

Cancel r? mainly for the issue when image load fails.

::: layout/style/nsStyleStruct.h:365
(Diff revision 1)
> +  mozilla::css::ImageValue* mImageValue;  // we can AddRef on an arbitrary
> +                                          // thread, but Release on main

I'd prefer using RefPtr here rather than manually maintaining the refcount.

::: layout/style/nsStyleStruct.cpp:1992
(Diff revision 1)
> +  if (!req) {
> +    // The URL resolution or image load failed.
> +    return false;
> +  }

When this happens, mResolved is true, while mRequestProxy is null. There would be null-deference if {Lock,Unlock}Image is called, and {Track,Untrack}Image would either assert or operate on an invalid pointer.

Should the callsites of those methods guarantee to only call them when .get() returns non-null? If yes, you probaly want an extra assertion in those methods. If no, you may want to add another branch to check mRequestProxy.

	Cameron McCormack (:heycam) Assignee
	Comment 16 • 8 years ago
mozreview-review-reply

Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.

https://reviewboard.mozilla.org/r/85106/#review83958

> I'd prefer using RefPtr here rather than manually maintaining the refcount.

Oh, NS_ReleaseOnMainThread(someRefPtrVariable.forget()) is an established pattern.  I'll use that.

> When this happens, mResolved is true, while mRequestProxy is null. There would be null-deference if {Lock,Unlock}Image is called, and {Track,Untrack}Image would either assert or operate on an invalid pointer.
> 
> Should the callsites of those methods guarantee to only call them when .get() returns non-null? If yes, you probaly want an extra assertion in those methods. If no, you may want to add another branch to check mRequestProxy.

That's true.  But in a later patch I will check the return value from Resolve(), and set an nsStyleImage (which stores an nsStyleImageRequest) from nsStyleImageType_Image to eStyleImageType_Null.  Still, probably better to null check mRequestProxy inside LockImage etc., in case we use nsStyleImageRequest in a different way for some other properties.

	Xidorn Quan [:xidorn] UTC+11
	Comment 17 • 8 years ago
mozreview-review

Comment on attachment 8800086 [details]
Bug 1288302 - Part 4: Perform final main thread work on style structs sourced from ServoComputedValues.

https://reviewboard.mozilla.org/r/85108/#review84018

	Xidorn Quan [:xidorn] UTC+11
	Comment 18 • 8 years ago
mozreview-review

Comment on attachment 8800087 [details]
Bug 1288302 - Part 5: Make nsStyleImage use nsStyleImageRequest.

https://reviewboard.mozilla.org/r/85110/#review83968

::: layout/style/nsRuleNode.cpp:140
(Diff revision 1)
> +static void
> +SetStyleImageRequest(function<void(nsStyleImageRequest*)> aCallback,
> +                     nsPresContext* aPresContext,
> +                     const nsCSSValue& aValue)

I don't really understand how does it make sense to use callback here? Wouldn't it be easier to make this function, as well as SetImageRequest, just return an already_AddRefed<_>?

::: layout/style/nsStyleStruct.h:898
(Diff revision 1)
> +    for (uint32_t i = 0; i < mImageCount; ++i)
> +      mLayers[i].ResolveImages(aContext);

Please wrap the loop body with {}.

::: layout/style/nsStyleStruct.cpp:2026
(Diff revision 1)
> +    aImageTracker->AddRef();
> +    nsMainThreadPtrHandle<imgRequestProxy> req = mRequestProxy;
> +    NS_DispatchToMainThread(NS_NewRunnableFunction([aImageTracker, req]() mutable {
> +      aImageTracker->Remove(req);
> +      aImageTracker->Release();
> +    }));

I'm concerned about the lifetime management here. Please create an nsRunnable for it rather than using lambda.

(Actually using lambda could be fine if we can use "initialized lambda captures". But we can't.)

	Cameron McCormack (:heycam) Assignee
	Comment 19 • 8 years ago

I found another problem with the design of nsStyleImageRequest, while trying to use it for list-style-image.  It can only buffer up a single call to TrackImage() before it's resolved.  But if we share the same nsStyleImageRequest object in multiple style structs (as will happen when inheriting the property), we'll try to call TrackImage() more than once on the same object.

So either this buffering up of TrackImage calls needs to be done at on the object that stores the RefPtr<nsStyleImageRequest>, or the nsStyleImageRequest itself needs to be able to count how many TrackImage() calls it's had before being resolved.  The latter would also need to assume that the ImageTracker object being used when finally acting on the buffered up TrackImage() calls is the same, no matter which style struct is the first one to resolve the nsStyleImageRequest.  Which is true, but it seems cleaner to do it separately from each struct.

	Cameron McCormack (:heycam) Assignee
	Comment 20 • 8 years ago
mozreview-review-reply

Comment on attachment 8800087 [details]
Bug 1288302 - Part 5: Make nsStyleImage use nsStyleImageRequest.

https://reviewboard.mozilla.org/r/85110/#review83968

> I don't really understand how does it make sense to use callback here? Wouldn't it be easier to make this function, as well as SetImageRequest, just return an already_AddRefed<_>?

Yeah, I'm not sure why I didn't convert the macros into that pattern.  Can I do that as a followup?

	Xidorn Quan [:xidorn] UTC+11
	Comment 21 • 8 years ago
mozreview-review-reply

Comment on attachment 8800087 [details]
Bug 1288302 - Part 5: Make nsStyleImage use nsStyleImageRequest.

https://reviewboard.mozilla.org/r/85110/#review83968

> Yeah, I'm not sure why I didn't convert the macros into that pattern.  Can I do that as a followup?

OK.

	Cameron McCormack (:heycam) Assignee
	Comment 30 • 8 years ago

Servo PR: https://github.com/servo/servo/pull/13786

	Xidorn Quan [:xidorn] UTC+11
	Comment 31 • 8 years ago
mozreview-review

Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.

https://reviewboard.mozilla.org/r/85106/#review84930

Mostly looks good to me. But I'm not very familiar with all the image things, so I'd want dholbert to check as well.

::: layout/style/nsStyleStruct.h:322
(Diff revision 2)
> +  // Mode describing whether LockImage/UnlockImage calls will be made
> +  // when obtaining and releasing the imgRequestProxy, and additionally
> +  // whether RequestDiscard will be called on release.
> +  enum class Mode : uint8_t {
> +    Locking,               // used by most nsStyleImageRequest
> +    LockingAndDiscarding,  // only used by nsCursorImage
> +    NonLocking,            // only used by nsStyleContentData
> +  };

It might be clearer to do it via flags, that is, having two flags, one for locking, and the other for requesting discard on destroy.

::: layout/style/nsStyleStruct.cpp:1970
(Diff revision 2)
> +  {
> +    RefPtr<StyleImageRequestCleanupTask> task =
> +        new StyleImageRequestCleanupTask(mLockingMode,
> +                                         mRequestProxy.forget(),
> +                                         mImageValue.forget(),
> +                                         mImageTracker.forget());

Should we probably just skip this block if mRequestProxy is nullptr and assert in the task that mRequestProxy is non-null?

It doesn't seem to make sense to create this cleanup task if we know it wouldn't do anything.

	Cameron McCormack (:heycam) Assignee
	Comment 32 • 8 years ago
mozreview-review-reply

Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.

https://reviewboard.mozilla.org/r/85106/#review84930

> It might be clearer to do it via flags, that is, having two flags, one for locking, and the other for requesting discard on destroy.

OK, I'll change this to use flags.  I realised yesterday that the mode for nsCursorImage is wrong, anyway, since for cursors we don't want to add them to the ImageTracker, but we still do want to lock/unlock/discard.

> Should we probably just skip this block if mRequestProxy is nullptr and assert in the task that mRequestProxy is non-null?
> 
> It doesn't seem to make sense to create this cleanup task if we know it wouldn't do anything.

We still need to release the mImageValue on the main thread, though.

	Xidorn Quan [:xidorn] UTC+11
	Comment 33 • 8 years ago
mozreview-review-reply

Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.

https://reviewboard.mozilla.org/r/85106/#review84930

> We still need to release the mImageValue on the main thread, though.

Oh, hmmm, yeah, you're right.

	Xidorn Quan [:xidorn] UTC+11
	Comment 34 • 8 years ago
mozreview-review-reply

Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.

https://reviewboard.mozilla.org/r/85106/#review84930

> OK, I'll change this to use flags.  I realised yesterday that the mode for nsCursorImage is wrong, anyway, since for cursors we don't want to add them to the ImageTracker, but we still do want to lock/unlock/discard.

Yeah, since document doesn't render it. That probably means you need three separate flags, then.

	Xidorn Quan [:xidorn] UTC+11
	Comment 35 • 8 years ago
mozreview-review

Comment on attachment 8800088 [details]
Bug 1288302 - Part 6: Add FFI function to set nsStyleImageRequest values.

https://reviewboard.mozilla.org/r/85112/#review84948

	Xidorn Quan [:xidorn] UTC+11
	Comment 36 • 8 years ago

Comment on attachment 8800087 [details]
Bug 1288302 - Part 5: Make nsStyleImage use nsStyleImageRequest.

Hmm, MozReview still doesn't handle redirecting review properly.

	Xidorn Quan [:xidorn] UTC+11
	Comment 37 • 8 years ago
mozreview-review

Comment on attachment 8800088 [details]
Bug 1288302 - Part 6: Add FFI function to set nsStyleImageRequest values.

https://reviewboard.mozilla.org/r/85112/#review84956

::: layout/style/ServoBindings.cpp:732
(Diff revision 2)
> +  RefPtr<nsStyleImageRequest> req =
> +    new nsStyleImageRequest(nsStyleImageRequest::Mode::Locking, urlBuffer,
> +                            do_AddRef(aBaseURI), do_AddRef(aReferrer),
> +                            do_AddRef(aPrincipal));
> +  aImage->SetImageRequest(req);

This makes me think nsStyleImage::SetImageRequest should probably take already_AddRefed<_> rather than a pointer.

	Xidorn Quan [:xidorn] UTC+11
	Comment 46 • 8 years ago
mozreview-review

Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.

https://reviewboard.mozilla.org/r/85106/#review85016

::: layout/style/nsStyleStruct.h:369
(Diff revision 3)
> +
> +private:
> +  ~nsStyleImageRequest();
> +  nsStyleImageRequest& operator=(const nsStyleImageRequest& aOther) = delete;
> +
> +  void TrackAndLock();

Probably better renaming this to MayTrackAndLock() given it doesn't always do that.

::: layout/style/nsStyleStruct.cpp:40
(Diff revision 3)
>  #include "mozilla/dom/AnimationEffectReadOnlyBinding.h" // for PlaybackDirection
>  #include "mozilla/dom/ImageTracker.h"
>  #include "mozilla/Likely.h"
>  #include "nsIURI.h"
>  #include "nsIDocument.h"
> +#include "nsNetUtil.h"

It doesn't seem to me any code you add in this file need this?

	Bobby Holley (:bholley)
	Comment 47 • 8 years ago
mozreview-review

Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.

https://reviewboard.mozilla.org/r/85106/#review85734

::: layout/style/nsStyleStruct.h:309
(Diff revision 3)
> + *    any thread.  The nsStyleImageRequest is not considered "resolved"
> + *    at this point, and the Resolve() method must be called later
> + *    to initiate the image load and make calls to get() valid.
> + *
> + * Calls to TrackImage(), UntrackImage(), LockImage(), UnlockImage() and
> + * RequestDiscard() are made to the imgRequestProxy as appropriate, according

nit: to the proxy and the tracker.

::: layout/style/nsStyleStruct.h:317
(Diff revision 3)
> + * The main thread constructor takes a pointer to the css::ImageValue that
> + * is the specified url() value, while the off-main-thread constructor
> + * creates a new css::ImageValue to represent the url() information passed
> + * to the constructor.  This ImageValue is held on to for the comparisons done
> + * in DefinitelyEquals(), so that we don't need to call into the non-OMT-safe
> + * imgRequestProxy::Equals().

I don't see imgRequestProxy::Equals anywhere. What is this referring to?

::: layout/style/nsStyleStruct.h:350
(Diff revision 3)
> +  const imgRequestProxy* get() const {
> +    MOZ_ASSERT(IsResolved(), "Resolve() must be called first");
> +    MOZ_ASSERT(NS_IsMainThread());
> +    return mRequestProxy.get();
> +  }

Nit: given the three lines it's probably nicer to const-cast-call the non-const method here.

::: layout/style/nsStyleStruct.h:361
(Diff revision 3)
> +    MOZ_ASSERT(IsResolved(), "Resolve() must be called first");
> +    MOZ_ASSERT(NS_IsMainThread());
> +    return mRequestProxy.get();
> +  }
> +
> +  bool DefinitelyEquals(const nsStyleImageRequest& aOther) const;

Would be good to add a comment indicating what kind of equality we're talking about here.

::: layout/style/nsStyleStruct.cpp:1910
(Diff revision 3)
> +    if (!mRequestProxy) {
> +      return NS_OK;
> +    }
> +
> +    MOZ_ASSERT(mImageTracker);

I'd rather just avoid instatiating and dispatching the cleanup task if these things are null.

::: layout/style/nsStyleStruct.cpp:2011
(Diff revision 3)
> +  nsCSSValue value;
> +  value.SetImageValue(mImageValue);
> +  mRequestProxy = value.GetImageValue(aPresContext->Document());
> +  if (!aPresContext->IsDynamic()) {
> +    mRequestProxy = nsContentUtils::GetStaticRequest(mRequestProxy);
> +  }

I'm a bit wary of duplicating this code from SetImageRequest. Can we find a way to share it? Or does it go away?

	Cameron McCormack (:heycam) Assignee
	Comment 48 • 8 years ago
mozreview-review-reply

Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.

https://reviewboard.mozilla.org/r/85106/#review85734

> I don't see imgRequestProxy::Equals anywhere. What is this referring to?

Oh, that should be something like "Equals on the nsIURI objects returned from imgRequestProxy::GetURI".

> I'd rather just avoid instatiating and dispatching the cleanup task if these things are null.

We always have a css::ImageValue object to release.

> I'm a bit wary of duplicating this code from SetImageRequest. Can we find a way to share it? Or does it go away?

It doesn't go away.  I'll find a way to share it.

	Bobby Holley (:bholley)
	Comment 49 • 8 years ago
mozreview-review

Comment on attachment 8800087 [details]
Bug 1288302 - Part 5: Make nsStyleImage use nsStyleImageRequest.

https://reviewboard.mozilla.org/r/85110/#review85746

::: layout/base/nsCSSRendering.cpp
(Diff revision 3)
> -  // We could do something fancy to avoid the TrackImage/UntrackImage
> -  // work, but it doesn't seem worth it.  (We need to call TrackImage
> -  // since we're not going through nsRuleNode::ComputeBorderData.)
> -  newStyleBorder.TrackImage(aPresContext->Document()->ImageTracker());

What replaces this work?

::: layout/style/nsComputedDOMStyle.cpp:2087
(Diff revision 3)
> +        // XXXheycam If we had some problem resolving the imgRequestProxy,
> +        // maybe we should just use the URL stored in the nsStyleImage's
> +        // mImageValue?

What does the spec say?

::: layout/style/nsRuleNode.cpp:144
(Diff revision 3)
> +                     nsStyleImageRequest::Mode aModeFlags =
> +                       nsStyleImageRequest::Mode::Track |
> +                       nsStyleImageRequest::Mode::Lock)

Are there any callers with the non-default for the last param? I don't see them in this patch.

::: layout/style/nsStyleStruct.h
(Diff revision 3)
> -#ifdef DEBUG
> -  bool mImageTracked;
> -#endif

Really glad to see these bools go, 7 years and a bit after I added them. ;-)

::: layout/style/nsStyleStruct.h:823
(Diff revision 3)
>      ~Layer();
>  
>      // Initialize mRepeat and mOrigin by specified layer type
>      void Initialize(LayerType aType);
>  
> -    // Register/unregister images with the document. We do this only
> +    void ResolveImages(nsPresContext* aContext) {

Seems like this should be (singular) ResolveImage?

	Cameron McCormack (:heycam) Assignee
	Comment 50 • 8 years ago
mozreview-review-reply

Comment on attachment 8800087 [details]
Bug 1288302 - Part 5: Make nsStyleImage use nsStyleImageRequest.

https://reviewboard.mozilla.org/r/85110/#review85746

> What replaces this work?

Nothing; this tracking was only to avoid an assertion that checked whether we had called TrackImage.  Now that we are sharing the same nsStyleImageRequest object, which itself has called TrackImage, we don't need to do anything.

> What does the spec say?

If the problem was just in producing the imgRequestProxy, I'm pretty sure we should still be returning the absolutized URL.  The spec says that if resolving a relative URL to an absolute URL fails, we should return the relative URL.  I'll file a followup bug to do this.  (The current Gecko behaviour in this case is to return "none".)

> Are there any callers with the non-default for the last param? I don't see them in this patch.

No, but is one in bug 1310560.

> Seems like this should be (singular) ResolveImage?

Yeah.  I was following the naming scheme of TrackImages and LockImages, which also on some objects only worked on a single image, but now that they are being removed I should fix this to be the singular.

	Pulsebot
	Comment 59 • 8 years ago

Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0488e9c0024
Part 1: Make css::ImageValue constructable OMT. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bf6ad588219
Part 2: Pass ImageTracker to style struct image tracking methods instead of nsPresContext. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dc27315f0b4
Part 3: Add nsStyleImageRequest. r=xidorn,bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb97ae080e7
Part 4: Perform final main thread work on style structs sourced from ServoComputedValues. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/daba6ffe8319
Part 5: Make nsStyleImage use nsStyleImageRequest. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/6861f02dadfa
Part 6: Add FFI function to set nsStyleImageRequest values. r=xidorn

	Carsten Book [:Tomcat]
	Comment 60 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a0488e9c0024
https://hg.mozilla.org/mozilla-central/rev/4bf6ad588219
https://hg.mozilla.org/mozilla-central/rev/4dc27315f0b4
https://hg.mozilla.org/mozilla-central/rev/cbb97ae080e7
https://hg.mozilla.org/mozilla-central/rev/daba6ffe8319
https://hg.mozilla.org/mozilla-central/rev/6861f02dadfa

	Cameron McCormack (:heycam) Assignee
	Comment 61 • 8 years ago

Backed out for the bug 1311921 Talos regressions: https://hg.mozilla.org/integration/mozilla-inbound/rev/7fd0f297a13d

	Cameron McCormack (:heycam) Assignee
	Comment 62 • 8 years ago

OK, think I've found the problem.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=4241b302fb8f&newProject=try&newRevision=5738016310e2&framework=1&showOnlyImportant=0

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c5486099fe132cd26bc274c543e4c5dc411a048

	Cameron McCormack (:heycam) Assignee
	Comment 63 • 8 years ago

I was basically calling LockImage() twice (once due to processing Mode::Track, once for Mode::Lock), which meant we never unlocked an image until the ImageTracker itself went away.

	Xidorn Quan [:xidorn] UTC+11
	Comment 64 • 8 years ago
mozreview-review

Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.

https://reviewboard.mozilla.org/r/85106/#review89224

::: layout/style/nsStyleStruct.cpp:1928
(Diff revision 4)
> +    if (mModeFlags & Mode::Discard) {
> +      mRequestProxy->RequestDiscard();
> +    }

Discard should probably be the last to call, because it is basically a no-op if there is still any lock, which means if anything does "Track | Discard", the discard may never take effect. (It seems we currently don't have anything does that, though.)

	Xidorn Quan [:xidorn] UTC+11
	Comment 65 • 8 years ago

(In reply to Cameron McCormack (:heycam) from comment #63)
> Created attachment 8806293 [details] [diff] [review]
> Part 5.1: Don't use Track and Lock flags together for background-images.
> 
> I was basically calling LockImage() twice (once due to processing
> Mode::Track, once for Mode::Lock), which meant we never unlocked an image
> until the ImageTracker itself went away.

I don't quite understand. Even if one uses Track, the image would still be automatically removed from ImageTracker when nsStyleImageRequest passes away, right? And if it is the last one which requests tracking the image, the image would also be unlocked by ImageTracker, wouldn't it? Then why did we never unlock the image?

	Cameron McCormack (:heycam) Assignee
	Comment 66 • 8 years ago

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #65)
> I don't quite understand. Even if one uses Track, the image would still be
> automatically removed from ImageTracker when nsStyleImageRequest passes
> away, right? And if it is the last one which requests tracking the image,
> the image would also be unlocked by ImageTracker, wouldn't it? Then why did
> we never unlock the image?

The only time we end up with the imgRequestProxy having its mLockCount go down to zero was when the ImageTracker went away (or if restyling removed the image).  This is because the nsStyleImageRequest itself did one lock, and the ImageTracker did another lock.  When we switch tabs, for example, we'll call ImageTracker::SetLockingState(false), to bulk-unlock all of the images in the document.  But since every CSS image had an additional lock held by the nsStyleImageRequest, we were never able to discard decoded image data until the image or the whole document went away.

So I believe this was just causing us to keep decoded image data around longer than we are currently.  I plotted some graphs of the RSS measurements after each page reload in the Tp5 test, and it did seem like the drops in memory were shifted a bit later.  The net effect being that the average memory usage over the entire run was higher.

	Cameron McCormack (:heycam) Assignee
	Comment 67 • 8 years ago
mozreview-review-reply

Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.

https://reviewboard.mozilla.org/r/85106/#review89224

> Discard should probably be the last to call, because it is basically a no-op if there is still any lock, which means if anything does "Track | Discard", the discard may never take effect. (It seems we currently don't have anything does that, though.)

Good point.  I'll fix that when re-landing.

	Xidorn Quan [:xidorn] UTC+11
	Comment 68 • 8 years ago

Talked with heycam on IRC, and it seems we hold all the style structs when a document is put into the bfcache. And the old patches of this bug makes style structs lock the images, which prevents their data from being destroyed, and thus the regression on memory usage.

The idea about tracking vs. locking for images in style data is from bug 512260, and we do not yet have any document about when an image should be tracked vs. locked by the style struct. Given that previously, only list-style-image and cursor lock the image, it is supposed that we lock images which are usually small, and track the rest.

This issue brings me an idea that we can probably put the mode flags in the type level, so it could be easier to reason about the behavior of each nsStyleImageRequest without investigating all paths it could be setup. But that could be done in a separate bug, I guess.

Actually I think it is possible that it isn't worth distinguishing between list-style-image, cursor, and other images. We can probably just drop the mode flag and do track on all images, and manually discard cursor image in the style struct rather than in nsStyleImageRequest. But that certainly should be done in a separate bug.

	Xidorn Quan [:xidorn] UTC+11
	Comment 69 • 8 years ago

Comment on attachment 8806293 [details] [diff] [review]
Part 5.1: Don't use Track and Lock flags together for background-images.

Review of attachment 8806293 [details] [diff] [review]:
-----------------------------------------------------------------

In addition to the comment below, you may need to update your part 6 as well.

::: layout/style/nsStyleStruct.h
@@ +327,3 @@
>    enum class Mode : uint8_t {
>      Track   = 0x1,  // used by all except nsCursorImage
> +    Lock    = 0x2,  // used only by nsStyleList

It seems to me we do lock for cursor image as well, not only list-style-image.

So if those two are exclusive to each other and we want either one anyway, should we make them one flag? It could probably just be Lock, and if the flag is not set, track it, and if it is set, lock it.

	Cameron McCormack (:heycam) Assignee
	Comment 70 • 8 years ago

Comment on attachment 8806293 [details] [diff] [review]
Part 5.1: Don't use Track and Lock flags together for background-images.

Review of attachment 8806293 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleStruct.h
@@ +327,3 @@
>    enum class Mode : uint8_t {
>      Track   = 0x1,  // used by all except nsCursorImage
> +    Lock    = 0x2,  // used only by nsStyleList

OK.  It might be a little less explicit, but would prevent passing in both mutually exclusive options.

Whatever mode flags are passed in, the image is going to be locked (since the ImageTracker will do locking).  So I think the flag we keep should be Track rather than Lock.  Lack of the Track flag would then do Locking itself, instead.

	Cameron McCormack (:heycam) Assignee
	Comment 72 • 8 years ago

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=4241b302fb8f&newProject=try&newRevision=1b831f4930a4&framework=1&showOnlyImportant=0

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46637c295dab4efaad5ca494849e0c3fb1cdd7ae&group_state=expanded

	Pulsebot
	Comment 73 • 8 years ago

Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e2645d5688d
Part 1: Make css::ImageValue constructable OMT. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c122cf760d7
Part 2: Pass ImageTracker to style struct image tracking methods instead of nsPresContext. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/731ccb5d2ce4
Part 3: Add nsStyleImageRequest. r=xidorn,bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/65583d15ca91
Part 4: Perform final main thread work on style structs sourced from ServoComputedValues. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/112b8aac9300
Part 5: Make nsStyleImage use nsStyleImageRequest. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/c213c3194aa8
Part 5.1: Merge Track and Lock flags so we don't set them together. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef55ee026e2
Part 6: Add FFI function to set nsStyleImageRequest values. r=xidorn

	Phil Ringnalda (:philor)
	Comment 74 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2e2645d5688d
https://hg.mozilla.org/mozilla-central/rev/8c122cf760d7
https://hg.mozilla.org/mozilla-central/rev/731ccb5d2ce4
https://hg.mozilla.org/mozilla-central/rev/65583d15ca91
https://hg.mozilla.org/mozilla-central/rev/112b8aac9300
https://hg.mozilla.org/mozilla-central/rev/c213c3194aa8
https://hg.mozilla.org/mozilla-central/rev/4ef55ee026e2