SkPDF: SkPDFGraphicState Lookup hashtabled

SkPDF: SkPDFGraphicState Lookup hashtabled

In Release, running `dm --src skp --config pdf`, I get a
speedup of about 1.2%.

SkPDFGraphicState class:
-   Holds the subset of SkPaint that maps to a PDF Graphics
    State
-   These fields are easily comparable, making hashtable
    comparisons easy.

SkPDFCanon:
-   findGraphicState() takes a SkPDFGraphicState, not a SkPaint
-   fGraphicStateRecords is a SkHashSet, not a SkTDArray
  
SkPDFGraphicState:
-   mode_for_pdf() replaces logic inside equivalent(), but is
    only called once per lookup.
-   emitObject() no longer modifies the SkPDFGraphicState to
    cache the SkPDFDict stucture.  (Since it is de-duped,
    this get no speedup).
-   Static Functions that don't use the canon return a plain
    SkPDFDict now.  No need for fPopulated.

SkTHash.h
-   SkHashSet::forall added

SkPDFDevice; SkPDFShader
-   Updated for new SkPDFGraphicState interface.

BUG=skia:3585
Committed: https://skia.googlesource.com/skia/+/be27a118c277af23377d38e9b3bfd3fcc276114f

[hal.canary, 2015-03-31 22:48:44]

halcanary@google.com changed reviewers:
+ mtklein@google.com

[hal.canary, 2015-03-31 22:48:45]

ptal

[mtklein, 2015-04-01 12:23:46]

https://codereview.chromium.org/1046293002/diff/40001/src/pdf/SkPDFGraphicSta...
File src/pdf/SkPDFGraphicState.cpp (right):

https://codereview.chromium.org/1046293002/diff/40001/src/pdf/SkPDFGraphicSta...
src/pdf/SkPDFGraphicState.cpp:109: SkPDFGraphicState* pdfGraphicState =
canon->findGraphicState(pdfPaint);
Uh, isn't this just logically incorrect now?  If the pdf paint doesn't store all
the information, how can we use it as a key to look up a SkPDFGraphicState*? 
Won't they be a one SkPDFPaint to many SkPDFGraphicState?  I'd think the key
would still have to be the full SkPaint.

https://codereview.chromium.org/1046293002/diff/40001/src/pdf/SkPDFGraphicSta...
File src/pdf/SkPDFGraphicState.h (right):

https://codereview.chromium.org/1046293002/diff/40001/src/pdf/SkPDFGraphicSta...
src/pdf/SkPDFGraphicState.h:22: class SkPDFPaint {
// SkPDFPaint is the subset of SkPaint that the PDF backend needs that is not
encoded in SkPDFGraphicsState's dictionary.

is that right?

https://codereview.chromium.org/1046293002/diff/40001/src/pdf/SkPDFGraphicSta...
src/pdf/SkPDFGraphicState.h:30: , fXferMode(0xFF) {}
I'd think we'd leave these uninitialized, or zero them, or fill them with
SkPaint's defaults.  Why choose these particular values?

https://codereview.chromium.org/1046293002/diff/40001/src/pdf/SkPDFGraphicSta...
src/pdf/SkPDFGraphicState.h:38: SkPDFPaint(const SkPDFPaint& p)
Maybe just = default ?

https://codereview.chromium.org/1046293002/diff/40001/src/pdf/SkPDFGraphicSta...
src/pdf/SkPDFGraphicState.h:51: SkPaint::Cap strokeCap() const { return
(SkPaint::Cap)fStrokeCap; }
Seems like we should update these enums with `: uint8_t` so they're always a
byte without going through casting hoops.  (Another CL.)

https://codereview.chromium.org/1046293002/diff/40001/src/pdf/SkPDFGraphicSta...
src/pdf/SkPDFGraphicState.h:120: return SkChecksum::Murmur3(&p, sizeof(p));
Let's go with SkGoodHash.  Same effect now, with a slightly more abstracted
intention.

https://codereview.chromium.org/1046293002/diff/40001/src/pdf/SkPDFGraphicSta...
src/pdf/SkPDFGraphicState.h:122: static const SkPDFPaint&
GetKey(SkPDFGraphicState* g) {
These are so far away from where we're using them in the table that it probably
makes sense to separate them out into SkPDFPaint::HashTraits or a HashTraits
struct right where the table lives

[hal.canary, 2015-04-01 16:18:18]

moved a lot of code around, simplified, but basic logic is the same.

https://codereview.chromium.org/1046293002/diff/40001/src/pdf/SkPDFGraphicSta...
File src/pdf/SkPDFGraphicState.cpp (right):

https://codereview.chromium.org/1046293002/diff/40001/src/pdf/SkPDFGraphicSta...
src/pdf/SkPDFGraphicState.cpp:109: SkPDFGraphicState* pdfGraphicState =
canon->findGraphicState(pdfPaint);
On 2015/04/01 12:23:46, mtklein wrote:
> Uh, isn't this just logically incorrect now?  If the pdf paint doesn't store
all
> the information, how can we use it as a key to look up a SkPDFGraphicState*? 
> Won't they be a one SkPDFPaint to many SkPDFGraphicState?  I'd think the key
> would still have to be the full SkPaint.

Nope.  The other information is handled by the SkPDFDevice.

https://codereview.chromium.org/1046293002/diff/40001/src/pdf/SkPDFGraphicSta...
File src/pdf/SkPDFGraphicState.h (right):

https://codereview.chromium.org/1046293002/diff/40001/src/pdf/SkPDFGraphicSta...
src/pdf/SkPDFGraphicState.h:22: class SkPDFPaint {
On 2015/04/01 12:23:46, mtklein wrote:
> // SkPDFPaint is the subset of SkPaint that the PDF backend needs that is not
> encoded in SkPDFGraphicsState's dictionary.
> 
> is that right?b

Kinda.  Let me clear it up with better comments.

https://codereview.chromium.org/1046293002/diff/40001/src/pdf/SkPDFGraphicSta...
src/pdf/SkPDFGraphicState.h:30: , fXferMode(0xFF) {}
On 2015/04/01 12:23:46, mtklein wrote:
> I'd think we'd leave these uninitialized, or zero them, or fill them with
> SkPaint's defaults.  Why choose these particular values?

Fixed.

https://codereview.chromium.org/1046293002/diff/40001/src/pdf/SkPDFGraphicSta...
src/pdf/SkPDFGraphicState.h:38: SkPDFPaint(const SkPDFPaint& p)
On 2015/04/01 12:23:46, mtklein wrote:
> Maybe just = default ?

Done.

https://codereview.chromium.org/1046293002/diff/40001/src/pdf/SkPDFGraphicSta...
src/pdf/SkPDFGraphicState.h:51: SkPaint::Cap strokeCap() const { return
(SkPaint::Cap)fStrokeCap; }
On 2015/04/01 12:23:46, mtklein wrote:
> Seems like we should update these enums with `: uint8_t` so they're always a
> byte without going through casting hoops.  (Another CL.)

Acknowledged.

https://codereview.chromium.org/1046293002/diff/40001/src/pdf/SkPDFGraphicSta...
src/pdf/SkPDFGraphicState.h:120: return SkChecksum::Murmur3(&p, sizeof(p));
On 2015/04/01 12:23:46, mtklein wrote:
> Let's go with SkGoodHash.  Same effect now, with a slightly more abstracted
> intention.

Done.

https://codereview.chromium.org/1046293002/diff/40001/src/pdf/SkPDFGraphicSta...
src/pdf/SkPDFGraphicState.h:122: static const SkPDFPaint&
GetKey(SkPDFGraphicState* g) {
On 2015/04/01 12:23:46, mtklein wrote:
> These are so far away from where we're using them in the table that it
probably
> makes sense to separate them out into SkPDFPaint::HashTraits or a HashTraits
> struct right where the table lives

Done.

[hal.canary, 2015-04-01 16:18:43]

Patchset #4 (id:60001) has been deleted

[hal.canary, 2015-04-01 19:32:55]

PTAL

[mtklein, 2015-04-01 20:05:38]

The CQ bit was checked by mtklein@google.com

[mtklein, 2015-04-01 20:05:39]

lgtm

https://codereview.chromium.org/1046293002/diff/120001/src/pdf/SkPDFCanon.cpp
File src/pdf/SkPDFCanon.cpp (right):

https://codereview.chromium.org/1046293002/diff/120001/src/pdf/SkPDFCanon.cpp...
src/pdf/SkPDFCanon.cpp:113: return ptr ? ptr->fPtr : NULL;
It does not seem safe to return a non-const pointer.  If you change that, you'll
change the value in the hash set, but not its location, breaking its invariants.

https://codereview.chromium.org/1046293002/diff/120001/src/pdf/SkPDFCanon.h
File src/pdf/SkPDFCanon.h (right):

https://codereview.chromium.org/1046293002/diff/120001/src/pdf/SkPDFCanon.h#n...
src/pdf/SkPDFCanon.h:60: SkPDFGraphicState* findGraphicState(SkPDFGraphicState&)
const;
It's unusual for us to have non-const refs as function arguments.  If we mutate
the arg, let's pass as a pointer... if not, const&.

https://codereview.chromium.org/1046293002/diff/120001/src/pdf/SkPDFCanon.h#n...
src/pdf/SkPDFCanon.h:80: template <typename T>
This doesn't really need to be templated, does it?

https://codereview.chromium.org/1046293002/diff/120001/src/pdf/SkPDFCanon.h#n...
src/pdf/SkPDFCanon.h:81: struct Wrap {
// Wrap lets us store pointers in the hash set, but compare them by value.

https://codereview.chromium.org/1046293002/diff/120001/src/pdf/SkPDFCanon.h#n...
src/pdf/SkPDFCanon.h:82: explicit Wrap(T* ptr = NULL) : fPtr(ptr) {}
Do we do anything with these pointers that isn't const?  Can we take and store
const T* ?

https://codereview.chromium.org/1046293002/diff/120001/src/pdf/SkPDFCanon.h#n...
src/pdf/SkPDFCanon.h:91: return SkGoodHash(w.fPtr->hash());
Seems like overkill to rehash a hash?

[commit-bot: I haz the power, 2015-04-01 20:05:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1046293002/140001

[commit-bot: I haz the power, 2015-04-01 20:12:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-01 20:12:34]

Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[hal.canary, 2015-04-01 20:18:04]

The CQ bit was checked by halcanary@google.com

[hal.canary, 2015-04-01 20:18:04]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/1046293002/#ps160001
(title: "2015-04-01 (Wednesday) 16:16:21 EDT")

[commit-bot: I haz the power, 2015-04-01 20:18:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1046293002/160001

[commit-bot: I haz the power, 2015-04-01 20:31:23]

Committed patchset #8 (id:160001) as
https://skia.googlesource.com/skia/+/be27a118c277af23377d38e9b3bfd3fcc276114f