Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2446643003:
Fix memory leak in FuzzGradients (Closed)
DescriptionFix memory leak in FuzzGradients
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2446643003
Committed: https://skia.googlesource.com/skia/+/840f12a721fe5f544e0a03ce1e4aca3ad18389f6
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use vector instead #
Total comments: 13
Patch Set 3 : More cleanup #
Total comments: 4
Patch Set 4 : Address nits #Patch Set 5 : include vector #Messages
Total messages: 22 (9 generated)
Description was changed from ========== Fix memory leak in FuzzGradients BUG=skia: ========== to ========== Fix memory leak in FuzzGradients BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2446643003 ==========
kjlubick@google.com changed reviewers: + mtklein@google.com
mtklein@chromium.org changed reviewers: + mtklein@chromium.org
https://codereview.chromium.org/2446643003/diff/1/fuzz/FuzzGradients.cpp File fuzz/FuzzGradients.cpp (right): https://codereview.chromium.org/2446643003/diff/1/fuzz/FuzzGradients.cpp#newc... fuzz/FuzzGradients.cpp:40: uint32_t t_count; Out of curiosity, what is the t_ prefix for? https://codereview.chromium.org/2446643003/diff/1/fuzz/FuzzGradients.cpp#newc... fuzz/FuzzGradients.cpp:86: std::unique_ptr<SkColor[]> colors; This seems fine, but we'd normally use an SkTDArray or std::vector. We usually only resort to manually memory management when we have to for performance... most of the time it's easier to just let a class with a friendlier interface manage it. Now, if performance really matters here, we should probably go all the way to just putting an array of MAX_COUNT of each on the stack.
https://codereview.chromium.org/2446643003/diff/1/fuzz/FuzzGradients.cpp File fuzz/FuzzGradients.cpp (right): https://codereview.chromium.org/2446643003/diff/1/fuzz/FuzzGradients.cpp#newc... fuzz/FuzzGradients.cpp:40: uint32_t t_count; On 2016/10/24 at 17:51:11, mtklein_C wrote: > Out of curiosity, what is the t_ prefix for? I don't recall. It's been awhile since I wrote this. Temp? https://codereview.chromium.org/2446643003/diff/1/fuzz/FuzzGradients.cpp#newc... fuzz/FuzzGradients.cpp:86: std::unique_ptr<SkColor[]> colors; On 2016/10/24 at 17:51:11, mtklein_C wrote: > This seems fine, but we'd normally use an SkTDArray or std::vector. We usually only resort to manually memory management when we have to for performance... most of the time it's easier to just let a class with a friendlier interface manage it. > > Now, if performance really matters here, we should probably go all the way to just putting an array of MAX_COUNT of each on the stack. Done. Vector used. I don't care about the performance too much.
https://codereview.chromium.org/2446643003/diff/20001/fuzz/FuzzGradients.cpp File fuzz/FuzzGradients.cpp (right): https://codereview.chromium.org/2446643003/diff/20001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:42: if (t_count == 1) { Why don't we fuzz count == 1? https://codereview.chromium.org/2446643003/diff/20001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:46: if (fuzz->remaining() < (1 + t_count * (sizeof(SkColor) + sizeof(SkScalar)))) { What's the 1 extra byte here for? It's probably a good idea to cut this code if possible. It's guarding later code, but it means you have to keep more state in your head implicitly. Won't the calls to next() just fail if this isn't true? https://codereview.chromium.org/2446643003/diff/20001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:50: SkColor c; It doesn't make things any faster or slower to put these out here, but it does make the code a little harder to read. It's easiest to read code that's as tightly scoped as possible. Let's do something like colors->clear(); pos ->clear(); for (int i = 0; i < count; i++) { SkColor c; SkScalar s; if (!fuzz->next(&c) || !fuzz->next(&s)) { return false; } colors->push_back(c); pos ->push_back(s); } if (count) { std::sort(pos->begin(), pos->end()); (*pos)[0] = 0; (*pos)[count-1] = 1; } https://codereview.chromium.org/2446643003/diff/20001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:52: for (uint32_t i = 0; i < t_count; i++) { ordinarily we'd write int instead of uint32_t here. uint32_t gives off a vibe of, this must be exactly 4 bytes. https://codereview.chromium.org/2446643003/diff/20001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:53: fuzz->next(&c); Don't we need to check that calls to next() succeed? Have you considered marking next() with SK_WARN_UNUSED_RESULT? https://codereview.chromium.org/2446643003/diff/20001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:64: std::sort(pos->begin(), pos->begin() + t_count); pos->begin() + t_count is also known as pos->end(). That's the more idomatic thing to write here.
https://codereview.chromium.org/2446643003/diff/20001/fuzz/FuzzGradients.cpp File fuzz/FuzzGradients.cpp (right): https://codereview.chromium.org/2446643003/diff/20001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:42: if (t_count == 1) { On 2016/10/24 at 19:26:35, mtklein_C wrote: > Why don't we fuzz count == 1? The API said >=2, so I followed that rule. Looking inside the code though, it does seem that there is code that activates if count == 1. I've removed it to see that we properly handle when users don't read the API https://codereview.chromium.org/2446643003/diff/20001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:46: if (fuzz->remaining() < (1 + t_count * (sizeof(SkColor) + sizeof(SkScalar)))) { On 2016/10/24 at 19:26:35, mtklein_C wrote: > What's the 1 extra byte here for? > > It's probably a good idea to cut this code if possible. It's guarding later code, but it means you have to keep more state in your head implicitly. Won't the calls to next() just fail if this isn't true? FWIW, the extra 1 byte is for static_cast<SkShader::TileMode>(fuzz->nextRangeU(0, 3)); I'll be reworking this, I don't know why it seemed like a good idea at the time. https://codereview.chromium.org/2446643003/diff/20001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:50: SkColor c; On 2016/10/24 at 19:26:35, mtklein_C wrote: > It doesn't make things any faster or slower to put these out here, but it does make the code a little harder to read. It's easiest to read code that's as tightly scoped as possible. > > Let's do something like > > colors->clear(); > pos ->clear(); > for (int i = 0; i < count; i++) { > SkColor c; > SkScalar s; > if (!fuzz->next(&c) || !fuzz->next(&s)) { > return false; > } > colors->push_back(c); > pos ->push_back(s); > } > if (count) { > std::sort(pos->begin(), pos->end()); > (*pos)[0] = 0; > (*pos)[count-1] = 1; > } Done. https://codereview.chromium.org/2446643003/diff/20001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:52: for (uint32_t i = 0; i < t_count; i++) { On 2016/10/24 at 19:26:35, mtklein_C wrote: > ordinarily we'd write int instead of uint32_t here. uint32_t gives off a vibe of, this must be exactly 4 bytes. Well, then I get a warning because t_count is uint32_t. What's the preferred way to deal with that? https://codereview.chromium.org/2446643003/diff/20001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:53: fuzz->next(&c); On 2016/10/24 at 19:26:35, mtklein_C wrote: > Don't we need to check that calls to next() succeed? Have you considered marking next() with SK_WARN_UNUSED_RESULT? They only wouldn't succeed if there isn't enough "randomness" in fuzz. Since I'm reworking the above guard, I will be checking that now. SK_WARN_UNUSED_RESULT added. https://codereview.chromium.org/2446643003/diff/20001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:64: std::sort(pos->begin(), pos->begin() + t_count); On 2016/10/24 at 19:26:35, mtklein_C wrote: > pos->begin() + t_count is also known as pos->end(). That's the more idomatic thing to write here. Done.
lgtm https://codereview.chromium.org/2446643003/diff/40001/fuzz/FuzzGradients.cpp File fuzz/FuzzGradients.cpp (right): https://codereview.chromium.org/2446643003/diff/40001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:45: *mode = static_cast<SkShader::TileMode>(fuzz->nextRangeU(0, 3)); This call consumes 4 bytes... https://codereview.chromium.org/2446643003/diff/40001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:60: (*pos)[count - 1] = 1; I was wondering about this. You want pos=0 for count=1, right? Maybe add a note? // Order matters if count==1. We want pos==0.
https://codereview.chromium.org/2446643003/diff/20001/fuzz/FuzzGradients.cpp File fuzz/FuzzGradients.cpp (right): https://codereview.chromium.org/2446643003/diff/20001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:52: for (uint32_t i = 0; i < t_count; i++) { On 2016/10/24 at 20:22:30, kjlubick wrote: > On 2016/10/24 at 19:26:35, mtklein_C wrote: > > ordinarily we'd write int instead of uint32_t here. uint32_t gives off a vibe of, this must be exactly 4 bytes. > > Well, then I get a warning because t_count is uint32_t. What's the preferred way to deal with that? This is why we always make things like this ints. Generally use int, but use size_t for byte lengths. uint32_t should only be used if something explicitly needs to be 4 bytes long (if, e.g you're breaking it up into 4 uint8_t using shifts). "unsigned" pretty much is never used.
On 2016/10/24 at 20:54:38, mtklein wrote: > https://codereview.chromium.org/2446643003/diff/20001/fuzz/FuzzGradients.cpp > File fuzz/FuzzGradients.cpp (right): > > https://codereview.chromium.org/2446643003/diff/20001/fuzz/FuzzGradients.cpp#... > fuzz/FuzzGradients.cpp:52: for (uint32_t i = 0; i < t_count; i++) { > On 2016/10/24 at 20:22:30, kjlubick wrote: > > On 2016/10/24 at 19:26:35, mtklein_C wrote: > > > ordinarily we'd write int instead of uint32_t here. uint32_t gives off a vibe of, this must be exactly 4 bytes. > > > > Well, then I get a warning because t_count is uint32_t. What's the preferred way to deal with that? > > This is why we always make things like this ints. Generally use int, but use size_t for byte lengths. uint32_t should only be used if something explicitly needs to be 4 bytes long (if, e.g you're breaking it up into 4 uint8_t using shifts). "unsigned" pretty much is never used. Good to know. When I rewrite the fuzzes to use next(T* n), I'll also add in a nextRange(T* n, T min, T max) so there's a way to do this for uint8_t, int and (if actually needed), uint32_t
The CQ bit was checked by kjlubick@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@chromium.org Link to the patchset: https://codereview.chromium.org/2446643003/#ps60001 (title: "Address nits")
https://codereview.chromium.org/2446643003/diff/40001/fuzz/FuzzGradients.cpp File fuzz/FuzzGradients.cpp (right): https://codereview.chromium.org/2446643003/diff/40001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:45: *mode = static_cast<SkShader::TileMode>(fuzz->nextRangeU(0, 3)); On 2016/10/24 at 20:52:23, mtklein_C wrote: > This call consumes 4 bytes... Whoops. https://codereview.chromium.org/2446643003/diff/40001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:60: (*pos)[count - 1] = 1; On 2016/10/24 at 20:52:23, mtklein_C wrote: > I was wondering about this. You want pos=0 for count=1, right? Maybe add a note? > > // Order matters if count==1. We want pos==0. Done.
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
Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
The CQ bit was checked by kjlubick@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@chromium.org Link to the patchset: https://codereview.chromium.org/2446643003/#ps80001 (title: "include vector")
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 ========== Fix memory leak in FuzzGradients BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2446643003 ========== to ========== Fix memory leak in FuzzGradients BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2446643003 Committed: https://skia.googlesource.com/skia/+/840f12a721fe5f544e0a03ce1e4aca3ad18389f6 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/840f12a721fe5f544e0a03ce1e4aca3ad18389f6 |
