Last Comment Bug 761393 - Cache gfxPatterns for gradients
: Cache gfxPatterns for gradients
Status: RESOLVED FIXED
[Snappy:P1]
:
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal with 1 vote (vote)
: mozilla18
Assigned To: Paul Adenot (:padenot)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 782459 973187
Blocks: 750871 785794 690623 australis-tabs-win
  Show dependency treegraph
 
Reported: 2012-06-04 15:00 PDT by Bas Schouten (:bas.schouten)
Modified: 2014-02-15 10:57 PST (History)
32 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Implements a cache for the CSS gradients. (23.95 KB, patch)
2012-07-05 20:56 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Part 2a - Implements an LRU cache policy. (4.21 KB, patch)
2012-07-05 20:58 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Part 2b - Implements an LFU cache policy. (5.01 KB, patch)
2012-07-05 21:05 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Part 3 - Add telemetry support. (4.14 KB, patch)
2012-07-09 18:18 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Add telemetry probes to have the hit rate. r= (4.97 KB, patch)
2012-07-10 17:16 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Add a method to hash an nsStyleCoord. r= (3.13 KB, patch)
2012-08-09 18:35 PDT, Paul Adenot (:padenot)
dbaron: review+
Details | Diff | Splinter Review
Cache the gfxPatterns using an nsExpirationTracker and an hashtable. r= (11.80 KB, patch)
2012-08-09 18:37 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Add telemetry probes to guess the retention time. r= (5.94 KB, patch)
2012-08-09 18:41 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Cache the gfxPatterns using an nsExpirationTracker and an hashtable. r= (11.27 KB, patch)
2012-08-10 09:44 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Add telemetry probes to guess the retention time. r= (6.12 KB, patch)
2012-08-10 11:33 PDT, Paul Adenot (:padenot)
taras.mozilla: review+
Details | Diff | Splinter Review
Add a method to hash an nsStyleCoord. r= (3.03 KB, patch)
2012-08-16 15:49 PDT, Paul Adenot (:padenot)
dbaron: review+
Details | Diff | Splinter Review
Cache the gfxPatterns using an nsExpirationTracker and an hashtable. r= (14.44 KB, patch)
2012-08-16 15:50 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Add telemetry probes to guess the retention time. r= (4.54 KB, patch)
2012-08-16 15:52 PDT, Paul Adenot (:padenot)
padenot: review+
Details | Diff | Splinter Review
Cache the gfxPatterns using an nsExpirationTracker and an hashtable. r= (16.03 KB, patch)
2012-08-16 20:58 PDT, Paul Adenot (:padenot)
dbaron: review-
Details | Diff | Splinter Review
Cache the gfxPatterns using an nsExpirationTracker and an hashtable. r= (33.84 KB, patch)
2012-08-22 10:55 PDT, Paul Adenot (:padenot)
dbaron: review+
Details | Diff | Splinter Review
Add a method to hash an nsStyleCoord. (3.04 KB, patch)
2012-08-26 21:09 PDT, Paul Adenot (:padenot)
padenot: review+
Details | Diff | Splinter Review
Cache the gfxPatterns using an nsExpirationTracker and an hashtable. (33.80 KB, patch)
2012-08-26 21:11 PDT, Paul Adenot (:padenot)
padenot: review+
Details | Diff | Splinter Review
Add telemetry probes to guess the retention time. (4.24 KB, patch)
2012-08-26 21:13 PDT, Paul Adenot (:padenot)
padenot: review+
Details | Diff | Splinter Review

Description Bas Schouten (:bas.schouten) 2012-06-04 15:00:08 PDT
Creating realizations of gradient stops for GPU accelerated drawing (and for Skia I believe CPU as well), is fairly expensive. We should probably cache the gradient stop realizations in a bunch of cases to improve performance. This is a matter of simply keeping the gfxPattern around for gradients.
Comment 1 Jet Villegas (:jet) 2012-06-05 15:48:45 PDT
Linear gradients are widely used as background images on popular sites. Example selectors from gmail.com that we could optimize for:

#lpt { background-image:-moz-linear-gradient(315deg,transparent,transparent 33%,rgba(0,0,0,0.12) 33%,rgba(0,0,0,0.12) 66%,transparent 66%,transparent); }

.gbqfb { background-image:-moz-linear-gradient(top,#4d90fe,#4787ed); }

.gbqfb-hvr:focus { background-image:-moz-linear-gradient(top,#4d90fe,#357ae8); }

.gbqfba { background-image:-moz-linear-gradient(top,#f5f5f5,#f1f1f1); }

.gbqfba-hvr:active { background-image:-moz-linear-gradient(top,#f8f8f8,#f1f1f1); }

.gbqfbb { background-image:-moz-linear-gradient(top,#fff,#fbfbfb); }

.gbqfbb-hvr,.gbqfbb-hvr:active { background-image:-moz-linear-gradient(top,#fff,#f8f8f8); }

.gbsbis .gbsbt { background-image:-moz-linear-gradient(top,rgba(0,0,0,.2),rgba(0,0,0,0)); }

.gbsb .gbsbb { background-image:-moz-linear-gradient(bottom,rgba(0,0,0,.2),rgba(0,0,0,0)); }
Comment 2 Bas Schouten (:bas.schouten) 2012-06-05 16:49:49 PDT
Yes, all these background images could probably just have their gfxPatterns cached. I'm not sure if Layout keeps its nsCSS-objects around. If it doesn't that would probably make things a little more tricky.
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2012-06-11 15:39:22 PDT
How much perf gain can we realize with this bug fixed?

We are currently using -moz-linear-gradient for the tabstrip on Firefox and are planning on moving away from computed gradients due to performance reasons.

Will fixing this bug make -moz-linear-gradient comparable to using repeated static images as backgrounds?
Comment 4 Bas Schouten (:bas.schouten) 2012-06-12 11:44:28 PDT
(In reply to Jared Wein [:jaws] from comment #3)
> How much perf gain can we realize with this bug fixed?

For D2D, a -lot-.

> We are currently using -moz-linear-gradient for the tabstrip on Firefox and
> are planning on moving away from computed gradients due to performance
> reasons.
> 
> Will fixing this bug make -moz-linear-gradient comparable to using repeated
> static images as backgrounds?

On D2D, yes, on software, it depends on the rendering backend. Static images will still be strictly faster, but obviously have other downsides.
Comment 5 Jet Villegas (:jet) 2012-06-29 06:37:51 PDT
Assigning to Paul as we discussed. Please see dholbert and bas for guidance here, as needed.
Comment 6 Paul Adenot (:padenot) 2012-07-05 20:56:39 PDT
Created attachment 639571 [details] [diff] [review]
Part 1 - Implements a cache for the CSS gradients.

This is WIP, and the cache policy is very naive: no upper bound on cache entries, no eviction.
Comment 7 Paul Adenot (:padenot) 2012-07-05 20:58:02 PDT
Created attachment 639572 [details] [diff] [review]
Part 2a - Implements an LRU cache policy.

This patch implements an LRU cache policy, with a maximum of 64 cache entry (about 1MB, as per Bas figures).
Comment 8 Paul Adenot (:padenot) 2012-07-05 21:05:16 PDT
Created attachment 639574 [details] [diff] [review]
Part 2b - Implements an LFU cache policy.

This (applied on 2a) implements an LFU cache policy instead, also with a 64 cache entries limit.

I'll setup a dev environment and benchmark both cache policies on Windows tomorrow.

I also plan to use a hashmap at some point (means coming up with a decent hash function for the nsStyleGradient, basically) instead of an array (again, needs profiling), and maybe to blend LFU and LRU (i.e. frequent gradient can be evicted if they have not been used in a long time), but maybe someone has a cool idea of what policy to adopt.

Finally, it would be useful to have a pref to set maximum number of cache entries.
Comment 9 (dormant account) 2012-07-06 20:18:29 PDT
telemetry seems like a good way to figure out good values for magic constants here
Comment 10 Paul Adenot (:padenot) 2012-07-09 18:18:50 PDT
Created attachment 640471 [details] [diff] [review]
Part 3 - Add telemetry support.

Taras devides a plan to get a sensible value for the number of cache entries we need. This patch makes the maximum size of the cache a random number between 0 and 511.

It also adds two telemetry probes, the first one tracking the size of the cache, the second one measuring the time (in milliseconds) taken by the gradient generation.

Hopefully we will be able to come up with a good value for the cache size, analyzing the data.

The first telemetry probe is name "GRADIENT_TABLE_SIZE" because if the name contains "CACHE", it gets picked up for a startup graph. It is not clear to me if we want that, since it duplicates the information. I might be missing something, though.
Comment 11 (dormant account) 2012-07-09 22:41:37 PDT
Comment on attachment 640471 [details] [diff] [review]
Part 3 - Add telemetry support.

+      mMaxEntries = random() % MAX_GRADIENT_CACHE_SIZE;
+      Telemetry::Accumulate(Telemetry::GRADIENT_TABLE_SIZE, mMaxEntries);
Max in telemetry is inclusive, so you are leaving out last bucket.  
mMaxEntries = random() % (MAX_GRADIENT_CACHE_SIZE+1);
Comment 12 (dormant account) 2012-07-09 22:44:20 PDT
+  Telemetry::AutoTimer<mozilla::Telemetry::GRADIENT_DURATION> timer;

are milliseconds sufficient here? IMAGE_DECODE_TIME uses microseconds
Comment 13 Paul Adenot (:padenot) 2012-07-10 17:16:48 PDT
Created attachment 640866 [details] [diff] [review]
Add telemetry probes to have the hit rate. r=

Taras, is about:telemetry supposed to work? I see different value from the
printfs I put in the code. 50-60us on average for a gradient on the printf,
while about:telemetry reports 7.5 on average.
Comment 14 (dormant account) 2012-07-11 11:50:06 PDT
(In reply to Paul ADENOT (:padenot) from comment #13)
> Created attachment 640866 [details] [diff] [review]
> Add telemetry probes to have the hit rate. r=
> 
> Taras, is about:telemetry supposed to work? I see different value from the
> printfs I put in the code. 50-60us on average for a gradient on the printf,
> while about:telemetry reports 7.5 on average.

http://mxr.mozilla.org/mozilla-central/source//ipc/chromium/src/base/histogram.cc should answer that q
Comment 15 Paul Adenot (:padenot) 2012-07-11 23:43:58 PDT
Taras, disregard comment 13, my objdir was in a bizarre state, a clobber fixed it, and about:telemetry reports good value.

Working on porting patch 1, 2a and 2b to use an hash map, instead of an array now.
Comment 16 (dormant account) 2012-07-19 15:52:40 PDT
(In reply to Paul ADENOT (:padenot) from comment #15)
> Taras, disregard comment 13, my objdir was in a bizarre state, a clobber
> fixed it, and about:telemetry reports good value.
> 
> Working on porting patch 1, 2a and 2b to use an hash map, instead of an
> array now.

Is this still being worked on?
Comment 17 Paul Adenot (:padenot) 2012-07-19 16:55:48 PDT
Yes, I've just been distracted by other things I'm working on.

I'm planning to talk to Bas tomorrow about that, since we are both in Toronto right now.
Comment 18 Paul Adenot (:padenot) 2012-07-31 15:34:20 PDT
I'm thinking of an hashtable + linked list solution here:

  hashtable<nsStyleGradient, {gfxPattern, linkedListElement*}>
  linkedlist<nsStyleGradient*>

The linked list is sorted in a most recently used fashion: When we hit the hashtable we take the likedListElement* which is part of the value, and we put it in front of the list.

When we want to evict a cache entry, we take the back of the list, and we remove this entry from the hashmap.

When we want to register a new entry in the cache, we add it at the front of the list, and we put it in the hashtable.

That gives us O(1) put, get, and eviction, and we have a MRU cache policy, at the cost of a few more pointers for the linkage scheme.

Any thought before I write the implementation?
Comment 19 Paul Adenot (:padenot) 2012-08-08 20:03:40 PDT
After a discussion with dbaron today, I'm going to try writing a solution based on an nsExpirationTracker and a Hashtable, instead of the LinkedList + Hashtable implementation, that I implemented today, but which has a lot of edge cases and is too complicated.

I plan to stop limiting the cache size, and relying on the nsExpirationTracker to remove entries. That might increase peak memory usage, but give the nice property to avoid retaining entries we don't use for a long time.
Comment 20 Paul Adenot (:padenot) 2012-08-09 18:35:03 PDT
Created attachment 650749 [details] [diff] [review]
Add a method to hash an nsStyleCoord. r=

This is required to be able to properly hash an nsStyleGradient.
Comment 21 Paul Adenot (:padenot) 2012-08-09 18:37:22 PDT
Created attachment 650750 [details] [diff] [review]
Cache the gfxPatterns using an nsExpirationTracker and an hashtable. r=

This actually implements the caching. The indentation is off a bit to avoid
adding a ton of whitespace changes to the patch. The retention time is arbitrar,
see next patch.
Comment 22 Paul Adenot (:padenot) 2012-08-09 18:41:32 PDT
Created attachment 650751 [details] [diff] [review]
Add telemetry probes to guess the retention time. r=

This randomizes the retention time. Telemetry probes are added to the mix to try
to get a sensible value for the retention time.
Comment 23 Bas Schouten (:bas.schouten) 2012-08-09 23:49:15 PDT
Comment on attachment 650750 [details] [diff] [review]
Cache the gfxPatterns using an nsExpirationTracker and an hashtable. r=

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

I don't seem to see the LinkedList system here yet. In any case this looks great in a quick overview. I'm not sure if I'm the right person to review this though. I'm not a layout peer and not familiar with the right usage of CSS and style system objects.

::: layout/style/nsStyleStruct.h
@@ +31,5 @@
>  #include "nsStyleTransformMatrix.h"
>  #include "nsAlgorithm.h"
>  #include "imgIRequest.h"
>  #include "gfxRect.h"
> +#include "mozilla/LinkedList.h"

Doesn't look to me like we need to drag this in in the header file.
Comment 24 Paul Adenot (:padenot) 2012-08-10 09:02:33 PDT
Bas, per discussion with dbaron and as explained in comment 18, I've switched away from the linked list/hashtable, to use the nsExpirationTracker class instead.

I'll fix the remaining LinkedList mention and flag someone appropriate for review.
Comment 25 Paul Adenot (:padenot) 2012-08-10 09:44:29 PDT
Created attachment 650915 [details] [diff] [review]
Cache the gfxPatterns using an nsExpirationTracker and an hashtable. r=

Removed the remaining LinkedList.h mention.
Comment 26 Paul Adenot (:padenot) 2012-08-10 11:33:59 PDT
Created attachment 650958 [details] [diff] [review]
Add telemetry probes to guess the retention time. r=

This changes the telemetry probes from:

    Maximum number of cache entry + Time to generate the gradients in us,

to:
    Retention time in ms + Time to generate the gradients in us.

Where the retention time is a random number, choosed when the cache is created,
that varies between 0 and 40 seconds.
Comment 27 David Baron :dbaron: ⌚️UTC-10 2012-08-10 13:44:46 PDT
Comment on attachment 650749 [details] [diff] [review]
Add a method to hash an nsStyleCoord. r=

I think I'd slightly prefer this being called HashValue() or HashCode() rather than GetHash().  (Though I think given the input parameter, HashValue() is probably better than HashCode().)

r=dbaron
Comment 28 David Baron :dbaron: ⌚️UTC-10 2012-08-10 14:00:03 PDT
Comment on attachment 650915 [details] [diff] [review]
Cache the gfxPatterns using an nsExpirationTracker and an hashtable. r=

>+    GradientCache(const PRUint32 cacheSize = 64)
>+      :nsExpirationTracker<GradientPatternCacheEntry, 4>(GENERATION_MS)
>+    {
>+      mHashEntries.Init();
>+    }

I think you should (a) call cacheSize aCacheSize and (b) actually pass it to Init().

(Otherwise waiting for the revised patch that you said is coming.)
Comment 29 Paul Adenot (:padenot) 2012-08-10 14:18:04 PDT
This parameter is a left over from the previous caching technique I used (fixed size + LRU). 

This will be removed in the next iteration, that will also fix the reftest failure and the Windows bustage I got on try (because calling |random()| obviously don't work on Windows).
Comment 30 (dormant account) 2012-08-13 15:51:23 PDT
Comment on attachment 650958 [details] [diff] [review]
Add telemetry probes to guess the retention time. r=

+    enum { MAX_GENERATION_MS = 10000};
Why is this better than const uint?

+    // We initialize the generation time to a random time (between 0 and 40s of
+    // retention), and time the gradient rendering against it.

Really 40s? Code reads more like 10s. One of these is wrong.

+    PRUint32 mTimerPeriod; <-- should be a const

+  TimeStamp start(TimeStamp::Now());
We have a raii class for this
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.h#62

+#define MAX_GRADIENT_CACHE_SIZE 512
Weren't we going to use telemetry to determine optimum size here?
Comment 31 Paul Adenot (:padenot) 2012-08-13 16:02:00 PDT
> +    enum { MAX_GENERATION_MS = 10000};
> Why is this better than const uint?

All other code that uses nsExpirationTracker uses an enum, so I used an enum for consistency.
I'll make it a const, which is nicer.

> 
> +    // We initialize the generation time to a random time (between 0 and 40s of
> +    // retention), and time the gradient rendering against it.
? 
> Really 40s? Code reads more like 10s. One of these is wrong.

Taras, the enum is the time for a generation. The template parameter in the nsExpirationTracker specifies we use 4 generations, hence the 40 seconds. I'll add a comment since it appears non-obvious.

> +#define MAX_GRADIENT_CACHE_SIZE 512
> Weren't we going to use telemetry to determine optimum size here?

After discussion with dbaron, I decided to ditch the max gradient cache size in favor of a time base eviction. I simply forgot to remove the #define, thanks for pointing that out.

> +  TimeStamp start(TimeStamp::Now());
> We have a raii class for this
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.h#62

The RAII class we have has a millisecond resolution, which is not sufficient and the reason why I'm doing this by hand.

I'll make the RAII class support either microseconds or milliseconds in the next iteration of the patch. Their is a couple occurrence in the code base that tracks the time in microseconds that could benefit from this new class.
Comment 32 (dormant account) 2012-08-13 16:25:22 PDT
Comment on attachment 650958 [details] [diff] [review]
Add telemetry probes to guess the retention time. r=

r+ if you do things you said in comment 31.

I'd prefer if the higher res autotimer was added in a followup
Comment 33 Paul Adenot (:padenot) 2012-08-13 16:37:08 PDT
Taras, I filed and assigned bug 782459 to myself for the followup.
Comment 34 Paul Adenot (:padenot) 2012-08-16 15:49:31 PDT
Created attachment 652600 [details] [diff] [review]
Add a method to hash an nsStyleCoord. r=

This fixes a mistake in the Calc case of the hashing function.
Comment 35 Paul Adenot (:padenot) 2012-08-16 15:50:20 PDT
Created attachment 652601 [details] [diff] [review]
Cache the gfxPatterns using an nsExpirationTracker and an hashtable. r=

This implements the feature. It's green on try.
Comment 36 Paul Adenot (:padenot) 2012-08-16 15:52:40 PDT
Created attachment 652603 [details] [diff] [review]
Add telemetry probes to guess the retention time. r=

* * *
Bug 782459 - Convert code that did the microseconds manually to the new RAII class. r=
Comment 37 Paul Adenot (:padenot) 2012-08-16 15:55:57 PDT
Comment on attachment 652603 [details] [diff] [review]
Add telemetry probes to guess the retention time. r=

Addressed comments, carrying forward r+. This need 782459 to work.
Comment 38 David Baron :dbaron: ⌚️UTC-10 2012-08-16 16:27:47 PDT
Comment on attachment 652600 [details] [diff] [review]
Add a method to hash an nsStyleCoord. r=

(In reply to Paul ADENOT (:padenot) from comment #34)
> This fixes a mistake in the Calc case of the hashing function.

I don't see how the change makes a difference either way, but r=dbaron.
Comment 39 David Baron :dbaron: ⌚️UTC-10 2012-08-16 16:33:33 PDT
Comment on attachment 652601 [details] [diff] [review]
Cache the gfxPatterns using an nsExpirationTracker and an hashtable. r=

>+struct GradientCacheKey {
>+  GradientCacheKey(nsRefPtr<nsStyleGradient> aGradient, const gfxRect& aOneCellArea)
>+    :mGradient(aGradient), mOneCellArea(aOneCellArea)
>+  {}
>+
>+  GradientCacheKey(const GradientCacheKey& aOther)
>+  {
>+    mGradient = aOther.mGradient;
>+    mOneCellArea = aOther.mOneCellArea;
>+  }
>+
>+  nsRefPtr<nsStyleGradient> mGradient;
>+  gfxRect mOneCellArea;

Would it make sense to cache a gfxSize instead of a gfxRect?  That seems to be what you're comparing.  (and gfxRect::Size() is a convenient way of getting one).

>+      // We don't cache gradient that have Calc value.
>+      if (aKey->HasCalc()) {
>+        return nullptr;
>+      }

Why not?

>+  if (pattern == nullptr) {
>   // Compute "gradient line" start and end relative to oneCellArea
>   gfxPoint lineStart, lineEnd;

Is this a whitespace-ignoring diff?  I hope so, though nothing in it seems to say so.
Comment 40 David Baron :dbaron: ⌚️UTC-10 2012-08-16 16:38:08 PDT
I also think a code comment explaining the relationship between GradientCacheKey, GradientCacheEntry, GradientCacheData, and GradientCache would make this a good bit easier to understand.  I'm having trouble following what's going on.
Comment 41 Paul Adenot (:padenot) 2012-08-16 16:54:27 PDT
(In reply to David Baron [:dbaron] from comment #39)
> Would it make sense to cache a gfxSize instead of a gfxRect?  That seems to
> be what you're comparing.  (and gfxRect::Size() is a convenient way of
> getting one).
It is indeed what I want to do, I was not aware that it existed.

> 
> >+      // We don't cache gradient that have Calc value.
> >+      if (aKey->HasCalc()) {
> >+        return nullptr;
> >+      }
> 
> Why not?

Pointer to Calc structs are guaranteed to outlive their nsStyleCoords, (reading the comment at http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleCoord.h#51). When we retain the gradients in this patch, we obviously change the life time of the nsStyleGradient. This makes the nsStyleCoord part of the nsStyleGradient sometimes outlive the Calc they have, that is deallocated because nsStyleCoord have weak references to Calc, and lead to crashes on Windows when running the test suite.

Hence, two solutions are available to us, here:
[1] Refcount the Calc, to restore the guarantee we have that the Calc pointer is valid ;
[2] Don't cache gradients that have Calc values.

I had initially written code to do [1], but I was not sure of the implications, since this code seems to have very careful allocation and deallocation patterns. The easy solution if to avoid caching them for now. If you feel that this is something we want, I can do [2] instead, but talking with Jet, it did not seem to be a priority. We might want to do it in a followup.

> 
> >+  if (pattern == nullptr) {
> >   // Compute "gradient line" start and end relative to oneCellArea
> >   gfxPoint lineStart, lineEnd;
> 
> Is this a whitespace-ignoring diff?  I hope so, though nothing in it seems
> to say so.

Since this patch added a whole level of indentation to bypass the computation of the data we cache, I intentionally messed up the indentation in the PaintGradient method to ease the review, as pointed in comment 21. Of course it will be fixed before landing. I agree I should simply have done a whitespace-ignoring.
Comment 42 David Baron :dbaron: ⌚️UTC-10 2012-08-16 17:30:33 PDT
(In reply to Paul ADENOT (:padenot) from comment #41)
> Pointer to Calc structs are guaranteed to outlive their nsStyleCoords,
> (reading the comment at
> http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleCoord.
> h#51). When we retain the gradients in this patch, we obviously change the
> life time of the nsStyleGradient. This makes the nsStyleCoord part of the
> nsStyleGradient sometimes outlive the Calc they have, that is deallocated
> because nsStyleCoord have weak references to Calc, and lead to crashes on
> Windows when running the test suite.
> 
> Hence, two solutions are available to us, here:
> [1] Refcount the Calc, to restore the guarantee we have that the Calc
> pointer is valid ;
> [2] Don't cache gradients that have Calc values.
> 
> I had initially written code to do [1], but I was not sure of the
> implications, since this code seems to have very careful allocation and
> deallocation patterns. The easy solution if to avoid caching them for now.
> If you feel that this is something we want, I can do [2] instead, but
> talking with Jet, it did not seem to be a priority. We might want to do it
> in a followup.

OK, that's fine, but the comment should explain why.
Comment 43 Paul Adenot (:padenot) 2012-08-16 20:58:18 PDT
Created attachment 652666 [details] [diff] [review]
Cache the gfxPatterns using an nsExpirationTracker and an hashtable. r=

Addresses comments, and add explanations about the class structure and the
ownership of different objects.
Comment 44 David Baron :dbaron: ⌚️UTC-10 2012-08-17 16:12:03 PDT
Comment on attachment 652666 [details] [diff] [review]
Cache the gfxPatterns using an nsExpirationTracker and an hashtable. r=

>+ * the nsStyleRect for the key, the gfxPattern for the value.

nsStyleRect -> nsStyleGradient and gfxRect
(or maybe just nsStyleGradient)




It occurs to me that it would be better to move the bulk of
GradientCacheEntry::HashKey into a HashKey method on nsStyleGradient,
so that people adding members to nsStyleGradient notice that code
needs to be added.

You should also remove the comment in GradientHashEntry::HashKey
about hashing X and Y, since you no longer have an X and Y.

Your style is rather inconsistent regarding indentation and spacing
around the ":" in constructor initializers.  Could you indent the ":"
two spaces throughout and also put a space after the ":" throughout.


The constructors for GradientCacheEntry and GradientCacheData
shouldn't take nsRefPtr<T> for parameters.  They should just take T*
instead.  Making them take nsRefPtr<T> causes extra reference counting.
Likewise undo the change in nsCSSRendering::PaintGradient.

And fix the indentation in nsCSSRendering::PaintGradient

I don't think GradientCache needs a virtual destructor.

I'm not crazy about the gradientRegistered business in
nsCSSRendering::PaintGradient, but it's probably not worth using
reference counting to fix it.


Why does GradientCacheEntry need to have a pointer to a
GradientCacheKey?
And why does GradientCacheData also need to have a copy of the
key (but not a pointer)?  (The comment says it has a strong reference,
but a copy isn't a strong reference.)
(This is part of what I was hoping to understand from the comments,
but they didn't help.  I'm thinking it's probably not needed.)

I'd like to look at this again, mainly because of the comments in the paragraph above.  I still don't understand why the setup with the hashtable and expiration tracker needs to be this complicated; I agree it needs to be a little complicated to deal with the expiration tracker tracking entries that need to be removed from a hashtable when they expire, but I don't think it needs to be this complicated.  Can the expiration tracker just contain a GradientCacheKey rather than GradientCacheEntry, and the GradientCacheEntry not contain or point to the key at all?

And maybe GradientCacheKey and GradientCacheEntry could both have names that end with "Key"?  The first is the conceptual key data (which needs to be in the expiration tracker) and the second -- which could perhaps just be a class that inherits from both PLDHashEntryHdr and GradientCacheKey -- is the key for an nsTHashtable.  (We usually use "Entry" in a pldhash context for a structure that contains both the key and the value, but what you're calling GradientCacheEntry is still really only the key.)
Comment 45 Paul Adenot (:padenot) 2012-08-22 10:55:38 PDT
Created attachment 654263 [details] [diff] [review]
Cache the gfxPatterns using an nsExpirationTracker and an hashtable. r=

(In reply to David Baron [:dbaron] from comment #44)
> I don't think GradientCache needs a virtual destructor.

This is not needed, but fixes a warning.

> Why does GradientCacheEntry need to have a pointer to a
> GradientCacheKey?
> And why does GradientCacheData also need to have a copy of the
> key (but not a pointer)?  (The comment says it has a strong reference,
> but a copy isn't a strong reference.)
> (This is part of what I was hoping to understand from the comments,
> but they didn't help.  I'm thinking it's probably not needed.)

I merged GradientCacheEntry and GradientCacheKey. The new class is called
GradientCacheKey, and is the key in the hash table. Hopefully this is less
confusing.

> Can the expiration tracker just contain a GradientCacheKey rather than
> GradientCacheEntry, and the GradientCacheEntry not contain or point to the key
> at all?

The GradientCacheEntry needs to have a key, because it's the class that will
hash it and compare it to make sure we found the right entry in the hash table.
From what I've seen in the tree, this seems to be the correct way to proceed.

GradientCacheData has to have a reference to the key, because it is the object
that is tracked by the nsExpirationTracker (I explain why we can't track
directly the key in the expiration tracker below). When the expiration tracker
notifies us that a certain entry has expired, we need the key to be able to
properly remove the entry in the hash table.

We can't make the expiration tracker track the key, because the key we have when
we do the lookup are temporary objects, and we would need the original one
tracked by the expiration tracker, since the expiration process rely on a state
on the tracked object.

Hence, we need to track the value of the hash map in the expiration tracker,
because we have the guarantee that we will have the same object each across
lookup, so that the expiration tracker can "age" and reset the timer of the
objects properly.
Comment 46 Daniel Holbert [:dholbert] 2012-08-22 11:04:52 PDT
(In reply to Paul ADENOT (:padenot) from comment #45)
> Created attachment 654263 [details] [diff] [review]
> Cache the gfxPatterns using an nsExpirationTracker and an hashtable. r=
> 
> (In reply to David Baron [:dbaron] from comment #44)
> > I don't think GradientCache needs a virtual destructor.
> 
> This is not needed, but fixes a warning.

You probably want to fix that warning by marking the class as MOZ_FINAL (promising that there are no subclasses of it, and hence no need for a virtual destructor), like so:
  class GradientCache MOZ_FINAL : public WhateverTheParentClassIs

(for reference, see bug 758992, where we did this in a bunch of other places to fix the same warning in existing code)
Comment 47 David Baron :dbaron: ⌚️UTC-10 2012-08-22 17:38:44 PDT
Comment on attachment 654263 [details] [diff] [review]
Cache the gfxPatterns using an nsExpirationTracker and an hashtable. r=

(In reply to comment 45)
>I merged GradientCacheEntry and GradientCacheKey. The new class is called
>GradientCacheKey, and is the key in the hash table. Hopefully this is less
>confusing.

Well, it's not great, since it's an extra word of storage per entry,
when I was hoping for less storage.

Now that I understand things again I realize that the way to get to less
storage (avoiding the extra two words of storage -- or now three words
with that revision -- of storing the key twice for each entry) would be
by using nsTHashtable directly (since the pldhash interface and the
nsTHashtable interface on top of it let the user store the key in the
entry; it's nsBaseHashtable on top of that that has a key+value entry.)

I'm ok with not doing that if you put in a FIXME comment suggesting it,
or I'm ok with you doing it now.

(Review of attachment 654263 [details] [diff] [review])
>+ * This class has a strong references to the data that is cached, and also a
>+ * strong reference to the key, to prevent both to be freed as long as they are
>+ * in the cache.

It doesn't have a strong reference to the data that are cached.  It *is*
the data that are cached.  The comment should say that, and that it
needs to be allocated as an object separate from the hashtable entry
so that the expiration tracker can track pointers to it.

That you're using nsRefPtr in the cached data doesn't need to be
explained in a comment, but if you feel the need to explain it,
you should say that the class has a strong reference to the
gradient and the gfxPattern.


And you should fix the warning as dholbert describes in comment 46.

r=dbaron with those things fixed, though I'm hoping you can switch to
using nsTHashtable directly, in which case:
 (1) you'd need Entry and Key classes again, but Entry would just
     derive from PLDHashEntryHdr and have nsAutoPtr<GradientCacheData>
 (2) Key would no longer derive from PLDHashEntryHdr
and I'd probably want to take one more look at the patch
Comment 48 Paul Adenot (:padenot) 2012-08-26 21:09:11 PDT
Created attachment 655496 [details] [diff] [review]
Add a method to hash an nsStyleCoord.

(rebased over PRInt change, carrying r+ forward).
Comment 49 Paul Adenot (:padenot) 2012-08-26 21:11:29 PDT
Created attachment 655498 [details] [diff] [review]
Cache the gfxPatterns using an nsExpirationTracker and an hashtable.

Addresses dbaron's comment. I'll address the nsClassHashtable -> nsTHashtable in
bug 785794. Carrying r+ forward.
Comment 50 Paul Adenot (:padenot) 2012-08-26 21:13:08 PDT
Created attachment 655500 [details] [diff] [review]
Add telemetry probes to guess the retention time.

(rebased over PRInt changes, carrying r+ forward).
Comment 53 "Saptarshi Guha[:joy]" 2012-11-17 12:40:32 PST
See https://bugzilla.mozilla.org/show_bug.cgi?id=811781#c8 for an update using Telemetry to choose GRADIENT_RETENTION_TIME.

Note You need to log in before you can comment on or make changes to this bug.