Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2488523003:
Make SkSmallAllocator obey the RAII invariants and be expandable (Closed)
DescriptionMake SkSmallAllocator obey the RAII invariants and move to heap structures when needed.
The biggest change is to the API which allowed code to bypass the
destruction invariants. This destruction bypass feature was needed in
only one use, and is totally encapsulated using createWithIniterT.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2488523003
Committed: https://skia.googlesource.com/skia/+/d5dc657b8c3ac916f98005dafdedafe02f023449
Committed: https://skia.googlesource.com/skia/+/c18b5f8f57a4efc5d5d1e399ed8bd3bd02c592ab
Committed: https://skia.googlesource.com/skia/+/bf6d80a7a496f96589f1592ee5f644002fbb9f6d
Patch Set 1 #Patch Set 2 : Use STArray for holding info #Patch Set 3 : fix test #Patch Set 4 : fix includes #
Total comments: 8
Patch Set 5 : Remove test problem. #Patch Set 6 : Desturctor name change #
Total comments: 1
Patch Set 7 : Make SkSmallAllocator enforce alignment #Patch Set 8 : Use older version of max_align_t #Patch Set 9 : Pick different alignments for different machines. #Patch Set 10 : Remove size fields #Patch Set 11 : Rewrite align and bookkeepping #Patch Set 12 : Fix debug build #Patch Set 13 : Remove the space field #
Total comments: 2
Patch Set 14 : Use less for comparison. #Patch Set 15 : Fix patch conflict. #Patch Set 16 : Strip pointer to make correct type #
Messages
Total messages: 98 (62 generated)
Description was changed from ========== Make SkSmallAllocator obey the RAII invariants and be expandable BUG=skia: ========== to ========== Make SkSmallAllocator obey the RAII invariants and be expandable BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2488523003 ==========
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Use STArray for holding info
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
Description was changed from ========== Make SkSmallAllocator obey the RAII invariants and be expandable BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2488523003 ========== to ========== Make SkSmallAllocator obey the RAII invariants and move to heap structures when needed. The biggest change is to the API which allowed code to bypass the destruction invariants. This destruction bypass feature was needed in only one use, and is totally encapsulated using createWithIniterT. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2488523003 ==========
herb@google.com changed reviewers: + bungeman@google.com
fix test
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
fix includes
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2488523003/diff/60001/src/core/SkSmallAllocat... File src/core/SkSmallAllocator.h (right): https://codereview.chromium.org/2488523003/diff/60001/src/core/SkSmallAllocat... src/core/SkSmallAllocator.h:79: struct Rec { If this Rec were to have a destructor which did the deleting / calling of fDestructor (the Rec owned the allocation) then ~SkSmallAllocator becomes trivial and freeLast becomes simple. On the other hand, this also means needing to split up the allocation and the setting of the fDestructor (the same way throwing from a class constructor doesn't call the destructor). https://codereview.chromium.org/2488523003/diff/60001/src/core/SkSmallAllocat... src/core/SkSmallAllocator.h:87: static void DestroyT(void* ptr) { Seems like this would now be less confusingly named 'DefaultDestructor'. https://codereview.chromium.org/2488523003/diff/60001/src/core/SkSmallAllocat... src/core/SkSmallAllocator.h:125: char fStorage[kTotalBytes]; Did we lose alignment here? Do we care about alignment in general (reserve doesn't seem to care). https://codereview.chromium.org/2488523003/diff/60001/tests/Float16Test.cpp File tests/Float16Test.cpp (right): https://codereview.chromium.org/2488523003/diff/60001/tests/Float16Test.cpp#n... tests/Float16Test.cpp:106: alternate = std::signbit(f) ? 0x8000 : 0x0000; This change seems unrelated?
Remove test problem.
Desturctor name change
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2488523003/diff/60001/src/core/SkSmallAllocat... File src/core/SkSmallAllocator.h (right): https://codereview.chromium.org/2488523003/diff/60001/src/core/SkSmallAllocat... src/core/SkSmallAllocator.h:79: struct Rec { On 2016/11/09 19:30:56, bungeman-skia wrote: > If this Rec were to have a destructor which did the deleting / calling of > fDestructor (the Rec owned the allocation) then ~SkSmallAllocator becomes > trivial and freeLast becomes simple. On the other hand, this also means needing > to split up the allocation and the setting of the fDestructor (the same way > throwing from a class constructor doesn't call the destructor). I will work on this in a follow up CL. https://codereview.chromium.org/2488523003/diff/60001/src/core/SkSmallAllocat... src/core/SkSmallAllocator.h:87: static void DestroyT(void* ptr) { On 2016/11/09 19:30:56, bungeman-skia wrote: > Seems like this would now be less confusingly named 'DefaultDestructor'. Done. https://codereview.chromium.org/2488523003/diff/60001/src/core/SkSmallAllocat... src/core/SkSmallAllocator.h:125: char fStorage[kTotalBytes]; On 2016/11/09 19:30:56, bungeman-skia wrote: > Did we lose alignment here? Do we care about alignment in general (reserve > doesn't seem to care). Yes, and this is the same behavior is the original code. I will fix this in a follow up CL. https://codereview.chromium.org/2488523003/diff/60001/tests/Float16Test.cpp File tests/Float16Test.cpp (right): https://codereview.chromium.org/2488523003/diff/60001/tests/Float16Test.cpp#n... tests/Float16Test.cpp:106: alternate = std::signbit(f) ? 0x8000 : 0x0000; On 2016/11/09 19:30:56, bungeman-skia wrote: > This change seems unrelated? Removed
lgtm with promises
Description was changed from ========== Make SkSmallAllocator obey the RAII invariants and move to heap structures when needed. The biggest change is to the API which allowed code to bypass the destruction invariants. This destruction bypass feature was needed in only one use, and is totally encapsulated using createWithIniterT. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2488523003 ========== to ========== Make SkSmallAllocator obey the RAII invariants and move to heap structures when needed. The biggest change is to the API which allowed code to bypass the destruction invariants. This destruction bypass feature was needed in only one use, and is totally encapsulated using createWithIniterT. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2488523003 ==========
mtklein@chromium.org changed reviewers: + mtklein@chromium.org
https://codereview.chromium.org/2488523003/diff/100001/src/core/SkSmallAlloca... File src/core/SkSmallAllocator.h (right): https://codereview.chromium.org/2488523003/diff/100001/src/core/SkSmallAlloca... src/core/SkSmallAllocator.h:53: T* createWithIniterT(size_t size, Initer initer) { Can't we infer T? template <typename Initer> auto createWithIniter(size_t, Initer initer) -> decltype(initer(nullptr)) { ... } ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/09 20:47:57, mtklein_C wrote: > https://codereview.chromium.org/2488523003/diff/100001/src/core/SkSmallAlloca... > File src/core/SkSmallAllocator.h (right): > > https://codereview.chromium.org/2488523003/diff/100001/src/core/SkSmallAlloca... > src/core/SkSmallAllocator.h:53: T* createWithIniterT(size_t size, Initer initer) > { > Can't we infer T? > > template <typename Initer> > auto createWithIniter(size_t, Initer initer) -> decltype(initer(nullptr)) { > ... > } > > ? I'll add it to the next CL. It is already in the works. Checking this one in.
The CQ bit was checked by herb@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make SkSmallAllocator obey the RAII invariants and move to heap structures when needed. The biggest change is to the API which allowed code to bypass the destruction invariants. This destruction bypass feature was needed in only one use, and is totally encapsulated using createWithIniterT. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2488523003 ========== to ========== Make SkSmallAllocator obey the RAII invariants and move to heap structures when needed. The biggest change is to the API which allowed code to bypass the destruction invariants. This destruction bypass feature was needed in only one use, and is totally encapsulated using createWithIniterT. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2488523003 Committed: https://skia.googlesource.com/skia/+/d5dc657b8c3ac916f98005dafdedafe02f023449 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/d5dc657b8c3ac916f98005dafdedafe02f023449
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2485853005/ by mtklein@chromium.org. The reason for reverting is: Crashing Mac Perf and Test bots. This is a flaky but extremely likely crash. I've only seen one Mac Perf or Test bot that had this patch that didn't crash. This should be easy to reproduce like this: $ gn gen out --args=is_debug=false $ ninja -C out dm $ out/dm -m xfermodes3 --config gpu This is crashing every time I run it on my laptop, and never when I revert this CL. Building in release and running --config gpu probably don't matter..
Message was sent while issue was closed.
Description was changed from ========== Make SkSmallAllocator obey the RAII invariants and move to heap structures when needed. The biggest change is to the API which allowed code to bypass the destruction invariants. This destruction bypass feature was needed in only one use, and is totally encapsulated using createWithIniterT. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2488523003 Committed: https://skia.googlesource.com/skia/+/d5dc657b8c3ac916f98005dafdedafe02f023449 ========== to ========== Make SkSmallAllocator obey the RAII invariants and move to heap structures when needed. The biggest change is to the API which allowed code to bypass the destruction invariants. This destruction bypass feature was needed in only one use, and is totally encapsulated using createWithIniterT. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2488523003 Committed: https://skia.googlesource.com/skia/+/d5dc657b8c3ac916f98005dafdedafe02f023449 ==========
Make SkSmallAllocator enforce alignment
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL at the alignment fix.
On 2016/11/10 16:12:57, herb_g wrote: > PTAL at the alignment fix. This fixes the bug Mike pointed out.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-x86_64-Release-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
Use older version of max_align_t
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
Pick different alignments for different machines.
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Remove size fields
Rewrite align and bookkeepping
Fix debug build
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Remove the space field
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2488523003/diff/240001/src/core/SkSmallAlloca... File src/core/SkSmallAllocator.h (right): https://codereview.chromium.org/2488523003/diff/240001/src/core/SkSmallAlloca... src/core/SkSmallAllocator.h:134: if (rec.fObj < fStorage || &fStorage[kTotalBytes] <= rec.fObj) { std::less
Use less for comparison.
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by herb@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bungeman@google.com Link to the patchset: https://codereview.chromium.org/2488523003/#ps260001 (title: "Use less for comparison.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for src/core/SkRasterPipelineBlitter.cpp:
While running git apply --index -p1;
error: patch failed: src/core/SkRasterPipelineBlitter.cpp:99
error: src/core/SkRasterPipelineBlitter.cpp: patch does not apply
Patch: src/core/SkRasterPipelineBlitter.cpp
Index: src/core/SkRasterPipelineBlitter.cpp
diff --git a/src/core/SkRasterPipelineBlitter.cpp
b/src/core/SkRasterPipelineBlitter.cpp
index
b8c0107e262fd916b28a52ea6051a2bb19ce2fac..4d1fecc483a8eee7884ab1afdd49a882d109ddc2
100644
--- a/src/core/SkRasterPipelineBlitter.cpp
+++ b/src/core/SkRasterPipelineBlitter.cpp
@@ -99,8 +99,7 @@ SkBlitter* SkRasterPipelineBlitter::Create(const SkPixmap&
dst,
paint.getBlendMode(),
paint_color(dst,
paint));
auto earlyOut = [&] {
- blitter->~SkRasterPipelineBlitter();
- alloc->freeLast();
+ alloc->deleteLast();
return nullptr;
};
Fix patch conflict.
The CQ bit was checked by herb@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bungeman@google.com Link to the patchset: https://codereview.chromium.org/2488523003/#ps280001 (title: "Fix patch conflict.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make SkSmallAllocator obey the RAII invariants and move to heap structures when needed. The biggest change is to the API which allowed code to bypass the destruction invariants. This destruction bypass feature was needed in only one use, and is totally encapsulated using createWithIniterT. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2488523003 Committed: https://skia.googlesource.com/skia/+/d5dc657b8c3ac916f98005dafdedafe02f023449 ========== to ========== Make SkSmallAllocator obey the RAII invariants and move to heap structures when needed. The biggest change is to the API which allowed code to bypass the destruction invariants. This destruction bypass feature was needed in only one use, and is totally encapsulated using createWithIniterT. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2488523003 Committed: https://skia.googlesource.com/skia/+/d5dc657b8c3ac916f98005dafdedafe02f023449 Committed: https://skia.googlesource.com/skia/+/c18b5f8f57a4efc5d5d1e399ed8bd3bd02c592ab ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://skia.googlesource.com/skia/+/c18b5f8f57a4efc5d5d1e399ed8bd3bd02c592ab
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:280001) has been created in https://codereview.chromium.org/2494353002/ by mtklein@chromium.org. The reason for reverting is: bots crashing / asserting.
Message was sent while issue was closed.
Strip pointer to make correct type
Message was sent while issue was closed.
Description was changed from ========== Make SkSmallAllocator obey the RAII invariants and move to heap structures when needed. The biggest change is to the API which allowed code to bypass the destruction invariants. This destruction bypass feature was needed in only one use, and is totally encapsulated using createWithIniterT. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2488523003 Committed: https://skia.googlesource.com/skia/+/d5dc657b8c3ac916f98005dafdedafe02f023449 Committed: https://skia.googlesource.com/skia/+/c18b5f8f57a4efc5d5d1e399ed8bd3bd02c592ab ========== to ========== Make SkSmallAllocator obey the RAII invariants and move to heap structures when needed. The biggest change is to the API which allowed code to bypass the destruction invariants. This destruction bypass feature was needed in only one use, and is totally encapsulated using createWithIniterT. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2488523003 Committed: https://skia.googlesource.com/skia/+/d5dc657b8c3ac916f98005dafdedafe02f023449 Committed: https://skia.googlesource.com/skia/+/c18b5f8f57a4efc5d5d1e399ed8bd3bd02c592ab ==========
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by herb@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bungeman@google.com Link to the patchset: https://codereview.chromium.org/2488523003/#ps300001 (title: "Strip pointer to make correct type")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make SkSmallAllocator obey the RAII invariants and move to heap structures when needed. The biggest change is to the API which allowed code to bypass the destruction invariants. This destruction bypass feature was needed in only one use, and is totally encapsulated using createWithIniterT. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2488523003 Committed: https://skia.googlesource.com/skia/+/d5dc657b8c3ac916f98005dafdedafe02f023449 Committed: https://skia.googlesource.com/skia/+/c18b5f8f57a4efc5d5d1e399ed8bd3bd02c592ab ========== to ========== Make SkSmallAllocator obey the RAII invariants and move to heap structures when needed. The biggest change is to the API which allowed code to bypass the destruction invariants. This destruction bypass feature was needed in only one use, and is totally encapsulated using createWithIniterT. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2488523003 Committed: https://skia.googlesource.com/skia/+/d5dc657b8c3ac916f98005dafdedafe02f023449 Committed: https://skia.googlesource.com/skia/+/c18b5f8f57a4efc5d5d1e399ed8bd3bd02c592ab Committed: https://skia.googlesource.com/skia/+/bf6d80a7a496f96589f1592ee5f644002fbb9f6d ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://skia.googlesource.com/skia/+/bf6d80a7a496f96589f1592ee5f644002fbb9f6d
Message was sent while issue was closed.
https://codereview.chromium.org/2488523003/diff/240001/src/core/SkSmallAlloca... File src/core/SkSmallAllocator.h (right): https://codereview.chromium.org/2488523003/diff/240001/src/core/SkSmallAlloca... src/core/SkSmallAllocator.h:134: if (rec.fObj < fStorage || &fStorage[kTotalBytes] <= rec.fObj) { On 2016/11/11 20:58:23, bungeman-skia wrote: > std::less Done.
Message was sent while issue was closed.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
