Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1790023003:
SkPDF: add drop() virtual to release resources early. (Closed)
|
Created:
4 years, 9 months ago by hal.canary Modified:
4 years, 9 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@skpdf-global Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkPDF: add drop() virtual to release resources early.
Call drop() after calling emitObject() on top-level objects. In Debug
mode, assert that each object is emited exactly once by asserting that
emitObject is never called after drop(). Same for addResources().
To make sure that top level objects don't get deleted prematurely,
SkPDFObjNumMap takes a reference.
Motivation: save RAM. Allow even earlier serialization with later
changes.
Also: Switch some SkTDArrays to SkTArrays.
BUG=skia:5087
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1790023003
Committed: https://skia.googlesource.com/skia/+/bae235eb07cdfeb6dd92efa2b2143fa9e91d9d04
Patch Set 1 #Patch Set 2 : 2016-03-12 (Saturday) 16:04:48 EST #Patch Set 3 : Record move fns #
Total comments: 24
Patch Set 4 : 2016-03-18 (Friday) 16:56:13 EDT #Patch Set 5 : 2016-03-18 (Friday) 17:17:54 EDT #Patch Set 6 : free → release #
Messages
Total messages: 37 (16 generated)
Description was changed from ========== SkPDF: add dump() virtual to release resources early. Call dump() after calling emitObject() on top-level objects. In Debug mode, assert that each object is emited exactly once by asserting that emitObject is never called after dump(). Same for addResources(). To make sure that top level objects don't get deleted prematurely, SkPDFObjNumMap takes a reference. Motivation: save RAM. Alow even earlier serialization with later changes. Also: Switch some SkTDArrays to SkTArrays. BUG=skia:5087 ========== to ========== SkPDF: add dump() virtual to release resources early. Call dump() after calling emitObject() on top-level objects. In Debug mode, assert that each object is emited exactly once by asserting that emitObject is never called after dump(). Same for addResources(). To make sure that top level objects don't get deleted prematurely, SkPDFObjNumMap takes a reference. Motivation: save RAM. Alow even earlier serialization with later changes. Also: Switch some SkTDArrays to SkTArrays. BUG=skia:5087 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1790023003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1790023003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
halcanary@google.com changed reviewers: + djsollen@google.com
please take a look. depends on http://crrev.com/1788263002 further changes will allow some objects to be serialized *before* SkDocument::close(), leading to potentially very large memory savings.
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1790023003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1790023003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1790023003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1790023003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
halcanary@google.com changed reviewers: + tomhudson@google.com
Adding tom.
https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFBitmap.cpp File src/pdf/SkPDFBitmap.cpp (right): https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFBitmap.cpp... src/pdf/SkPDFBitmap.cpp:385: PDFAlphaBitmap(const SkImage* image) : fImage(SkRef(image)) { SkASSERT(image); } You don't want to do the force-to-be-nonnull-by-passing-in-a-ref thing here? https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFBitmap.cpp... src/pdf/SkPDFBitmap.cpp:386: ~PDFAlphaBitmap() {} Since the base class has virtual functions, do we still need to be declaring the destructor virtual? https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFBitmap.cpp... src/pdf/SkPDFBitmap.cpp:423: : fImage(SkRef(image)), fSMask(smask) { SkASSERT(fImage); } No destructor; do we need one since we've got virtuals? https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.cpp File src/pdf/SkPDFTypes.cpp (left): https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.cpp#... src/pdf/SkPDFTypes.cpp:430: void SkPDFDict::clear() { What happened to clear()? Do we just not need to call the union destructor any more? https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.cpp File src/pdf/SkPDFTypes.cpp (right): https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.cpp#... src/pdf/SkPDFTypes.cpp:278: // TODO: implement SkTArray<T>::reserve() TODO! If you intend to leave undone, is there anything somebody else needs to know to do this themselves, or a bug to refer to? https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.cpp#... src/pdf/SkPDFTypes.cpp:396: SkPDFDict::Record& SkPDFDict::Record::operator=(SkPDFDict::Record&& o) { You want move assignment but no non-destructive non-moving assignment? https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.cpp#... src/pdf/SkPDFTypes.cpp:528: fObjects.emplace_back(sk_ref_sp(obj)); You're ref'ing an sp, but input is a raw pointer?
mtklein@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.h File src/pdf/SkPDFTypes.h (right): https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.h#ne... src/pdf/SkPDFTypes.h:59: virtual void dump() {} We usually use dump() to mean, print this to stdout for debugging. Might want to rename to something else, like drop()?
https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.h File src/pdf/SkPDFTypes.h (right): https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.h#ne... src/pdf/SkPDFTypes.h:59: virtual void dump() {} On 2016/03/18 14:03:26, mtklein wrote: > We usually use dump() to mean, print this to stdout for debugging. > > Might want to rename to something else, like drop()? Yes! I was concerned but didn't have a better idea to suggest. drop() or some other alternative is better than dump in the context of the preexisting Skia patterns.
https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFBitmap.cpp File src/pdf/SkPDFBitmap.cpp (right): https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFBitmap.cpp... src/pdf/SkPDFBitmap.cpp:385: PDFAlphaBitmap(const SkImage* image) : fImage(SkRef(image)) { SkASSERT(image); } On 2016/03/18 13:59:38, tomhudson wrote: > You don't want to do the force-to-be-nonnull-by-passing-in-a-ref thing here? We rarely ever refer to SkImage types by reference. later, we'll pass it in as a sk_sp, anyways. https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFBitmap.cpp... src/pdf/SkPDFBitmap.cpp:386: ~PDFAlphaBitmap() {} On 2016/03/18 13:59:38, tomhudson wrote: > Since the base class has virtual functions, do we still need to be declaring the > destructor virtual? it's a final class https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFBitmap.cpp... src/pdf/SkPDFBitmap.cpp:423: : fImage(SkRef(image)), fSMask(smask) { SkASSERT(fImage); } On 2016/03/18 13:59:38, tomhudson wrote: > No destructor; do we need one since we've got virtuals? it's a final class. https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.cpp File src/pdf/SkPDFTypes.cpp (left): https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.cpp#... src/pdf/SkPDFTypes.cpp:430: void SkPDFDict::clear() { On 2016/03/18 13:59:38, tomhudson wrote: > What happened to clear()? Do we just not need to call the union destructor any > more? clear() is superceded by drop(), which has the same functionality, but is also virtual. https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.cpp File src/pdf/SkPDFTypes.cpp (right): https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.cpp#... src/pdf/SkPDFTypes.cpp:278: // TODO: implement SkTArray<T>::reserve() On 2016/03/18 13:59:38, tomhudson wrote: > TODO! If you intend to leave undone, is there anything somebody else needs to > know to do this themselves, or a bug to refer to? I put more info in the TODO. I haven't had time to explore this yet. https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.cpp#... src/pdf/SkPDFTypes.cpp:396: SkPDFDict::Record& SkPDFDict::Record::operator=(SkPDFDict::Record&& o) { On 2016/03/18 13:59:38, tomhudson wrote: > You want move assignment but no non-destructive non-moving assignment? Yep. I have not needed those yet, so I don't define them. https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.cpp#... src/pdf/SkPDFTypes.cpp:528: fObjects.emplace_back(sk_ref_sp(obj)); On 2016/03/18 13:59:38, tomhudson wrote: > You're ref'ing an sp, but input is a raw pointer? yes. this is awkward now and I will fix it later. I need to start holding a ref so that objects don't get unrefed out of existance when I call drop() uptree. https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.h File src/pdf/SkPDFTypes.h (right): https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.h#ne... src/pdf/SkPDFTypes.h:59: virtual void dump() {} On 2016/03/18 14:03:26, mtklein wrote: > We usually use dump() to mean, print this to stdout for debugging. > > Might want to rename to something else, like drop()? done. I like drop better. https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.h#ne... src/pdf/SkPDFTypes.h:59: virtual void dump() {} On 2016/03/18 14:11:54, tomhudson wrote: > On 2016/03/18 14:03:26, mtklein wrote: > > We usually use dump() to mean, print this to stdout for debugging. > > > > Might want to rename to something else, like drop()? > > Yes! I was concerned but didn't have a better idea to suggest. drop() or some > other alternative is better than dump in the context of the preexisting Skia > patterns. Done.
Description was changed from ========== SkPDF: add dump() virtual to release resources early. Call dump() after calling emitObject() on top-level objects. In Debug mode, assert that each object is emited exactly once by asserting that emitObject is never called after dump(). Same for addResources(). To make sure that top level objects don't get deleted prematurely, SkPDFObjNumMap takes a reference. Motivation: save RAM. Alow even earlier serialization with later changes. Also: Switch some SkTDArrays to SkTArrays. BUG=skia:5087 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkPDF: add drop() virtual to release resources early. Call drop() after calling emitObject() on top-level objects. In Debug mode, assert that each object is emited exactly once by asserting that emitObject is never called after drop(). Same for addResources(). To make sure that top level objects don't get deleted prematurely, SkPDFObjNumMap takes a reference. Motivation: save RAM. Allow even earlier serialization with later changes. Also: Switch some SkTDArrays to SkTArrays. BUG=skia:5087 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFBitmap.cpp File src/pdf/SkPDFBitmap.cpp (right): https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFBitmap.cpp... src/pdf/SkPDFBitmap.cpp:386: ~PDFAlphaBitmap() {} On 2016/03/18 21:18:07, Hal Canary wrote: > On 2016/03/18 13:59:38, tomhudson wrote: > > Since the base class has virtual functions, do we still need to be declaring > the > > destructor virtual? > > it's a final class This assertion is not sufficient for me to evaluate the accuracy of the design decision. A google search leads me to e.g. http://programmers.stackexchange.com/questions/284561/when-not-to-use-virtual..., but that's vague/self-contradictory enough that I don't really think it suffices for me to evaluate. The *base class* destructor has to be virtual, right? We don't really mark derived class destructors as override. We do want this destructor called if the base class pointer is deleted. So it should be virtual?! https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.cpp File src/pdf/SkPDFTypes.cpp (left): https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.cpp#... src/pdf/SkPDFTypes.cpp:430: void SkPDFDict::clear() { On 2016/03/18 21:18:07, Hal Canary wrote: > On 2016/03/18 13:59:38, tomhudson wrote: > > What happened to clear()? Do we just not need to call the union destructor any > > more? > > clear() is superceded by drop(), which has the same functionality, but is also > virtual. Acknowledged. https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.cpp File src/pdf/SkPDFTypes.cpp (right): https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.cpp#... src/pdf/SkPDFTypes.cpp:396: SkPDFDict::Record& SkPDFDict::Record::operator=(SkPDFDict::Record&& o) { On 2016/03/18 21:18:07, Hal Canary wrote: > On 2016/03/18 13:59:38, tomhudson wrote: > > You want move assignment but no non-destructive non-moving assignment? > > Yep. I have not needed those yet, so I don't define them. Acknowledged. https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFTypes.cpp#... src/pdf/SkPDFTypes.cpp:528: fObjects.emplace_back(sk_ref_sp(obj)); On 2016/03/18 21:18:07, Hal Canary wrote: > On 2016/03/18 13:59:38, tomhudson wrote: > > You're ref'ing an sp, but input is a raw pointer? > > yes. this is awkward now and I will fix it later. I need to start holding a > ref so that objects don't get unrefed out of existance when I call drop() > uptree. Plz to file bug or TODO about laters.
https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFBitmap.cpp File src/pdf/SkPDFBitmap.cpp (right): https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFBitmap.cpp... src/pdf/SkPDFBitmap.cpp:386: ~PDFAlphaBitmap() {} On 2016/03/21 13:54:30, tomhudson wrote: > On 2016/03/18 21:18:07, Hal Canary wrote: > > On 2016/03/18 13:59:38, tomhudson wrote: > > > Since the base class has virtual functions, do we still need to be declaring > > the > > > destructor virtual? > > > > it's a final class > > This assertion is not sufficient for me to evaluate the accuracy of the design > decision. A google search leads me to e.g. > http://programmers.stackexchange.com/questions/284561/when-not-to-use-virtual..., > but that's vague/self-contradictory enough that I don't really think it suffices > for me to evaluate. > > The *base class* destructor has to be virtual, right? We don't really mark > derived class destructors as override. We do want this destructor called if the > base class pointer is deleted. So it should be virtual?! Marking a class final prevents subclassing and also marks all its virtual methods final, as if we'd written ~PDFAlphaBitmap() final {} ... void dump() final { ... } etc. When written explicitly, final is similar to override, in that it won't compile unless the method was virtual. So neither void foo() final { ... } void bar() override { ... } would compile here because foo() and bar() are not virtual methods on the parent class. In effect, we have marked ~(), emitObject() and dump() all as final, and explicitly required that emitObject() and dump() override virtual methods. We could conceivably de-virtualize SkPDFObject's destructor and this code would continue to compile, but it'd leak fImage. For consistency, I'd suggest marking the destructor as override. It doesn't change anything except require SkPDFObject's destructor stays virtual. Marking destructors as override is a good habit to get into generally. https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFBitmap.cpp... src/pdf/SkPDFBitmap.cpp:423: : fImage(SkRef(image)), fSMask(smask) { SkASSERT(fImage); } On 2016/03/18 13:59:38, tomhudson wrote: > No destructor; do we need one since we've got virtuals? (No _explicit_ destructor. There is an implicitly-defined destructor here, and it does get called when deleting from a base pointer as long as that base pointer's destructor is virtual. Same story as above... if we want to require SkPDFObject's destructor is virtual, we could write ~PDFDefaultBitmap override {}.)
On 2016/03/21 14:11:15, mtklein wrote: > https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFBitmap.cpp > File src/pdf/SkPDFBitmap.cpp (right): > > https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFBitmap.cpp... > src/pdf/SkPDFBitmap.cpp:386: ~PDFAlphaBitmap() {} > On 2016/03/21 13:54:30, tomhudson wrote: > > On 2016/03/18 21:18:07, Hal Canary wrote: > > > On 2016/03/18 13:59:38, tomhudson wrote: > > > > Since the base class has virtual functions, do we still need to be > declaring > > > the > > > > destructor virtual? > > > > > > it's a final class > > > > This assertion is not sufficient for me to evaluate the accuracy of the design > > decision. A google search leads me to e.g. > > > http://programmers.stackexchange.com/questions/284561/when-not-to-use-virtual..., > > but that's vague/self-contradictory enough that I don't really think it > suffices > > for me to evaluate. > > > > The *base class* destructor has to be virtual, right? We don't really mark > > derived class destructors as override. We do want this destructor called if > the > > base class pointer is deleted. So it should be virtual?! > > Marking a class final prevents subclassing and also marks all its virtual > methods final, as if we'd written > ~PDFAlphaBitmap() final {} > ... > void dump() final { ... } > etc. > > When written explicitly, final is similar to override, in that it won't compile > unless the method was virtual. So neither > void foo() final { ... } > void bar() override { ... } > would compile here because foo() and bar() are not virtual methods on the parent > class. > > In effect, we have marked ~(), emitObject() and dump() all as final, and > explicitly required that emitObject() and dump() override virtual methods. We > could conceivably de-virtualize SkPDFObject's destructor and this code would > continue to compile, but it'd leak fImage. > > For consistency, I'd suggest marking the destructor as override. It doesn't > change anything except require SkPDFObject's destructor stays virtual. Marking > destructors as override is a good habit to get into generally. > > https://codereview.chromium.org/1790023003/diff/40001/src/pdf/SkPDFBitmap.cpp... > src/pdf/SkPDFBitmap.cpp:423: : fImage(SkRef(image)), fSMask(smask) { > SkASSERT(fImage); } > On 2016/03/18 13:59:38, tomhudson wrote: > > No destructor; do we need one since we've got virtuals? > > (No _explicit_ destructor. There is an implicitly-defined destructor here, and > it does get called when deleting from a base pointer as long as that base > pointer's destructor is virtual. Same story as above... if we want to require > SkPDFObject's destructor is virtual, we could write ~PDFDefaultBitmap override > {}.) Thanks, Mike. It feels like patching over all the holes in my modern C++ knowledge is a one-brick-at-a-time problem.
lgtm
> Thanks, Mike. It feels like patching over all the holes in my modern C++ > knowledge is a one-brick-at-a-time problem. Same here. The thing I try to remember for this stuff is that where we used to make chains like this: virtual -> virtual -> virtual -> virtual they're now best virtual -> override -> override -> final (The old style still works, with less constraint.)
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1790023003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1790023003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tomhudson@google.com Link to the patchset: https://codereview.chromium.org/1790023003/#ps100001 (title: "free → release")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1790023003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1790023003/100001
Message was sent while issue was closed.
Description was changed from ========== SkPDF: add drop() virtual to release resources early. Call drop() after calling emitObject() on top-level objects. In Debug mode, assert that each object is emited exactly once by asserting that emitObject is never called after drop(). Same for addResources(). To make sure that top level objects don't get deleted prematurely, SkPDFObjNumMap takes a reference. Motivation: save RAM. Allow even earlier serialization with later changes. Also: Switch some SkTDArrays to SkTArrays. BUG=skia:5087 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkPDF: add drop() virtual to release resources early. Call drop() after calling emitObject() on top-level objects. In Debug mode, assert that each object is emited exactly once by asserting that emitObject is never called after drop(). Same for addResources(). To make sure that top level objects don't get deleted prematurely, SkPDFObjNumMap takes a reference. Motivation: save RAM. Allow even earlier serialization with later changes. Also: Switch some SkTDArrays to SkTArrays. BUG=skia:5087 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/bae235eb07cdfeb6dd92efa2b2143fa9e91d9d04 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/bae235eb07cdfeb6dd92efa2b2143fa9e91d9d04 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
