Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
126743003:
Convert Checksum test to DEF_TEST() macro. (Closed)
|
Created:
6 years, 11 months ago by tfarina Modified:
6 years, 11 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionConvert Checksum test to DEF_TEST() macro.
BUG=None
TEST=tests
R=mtklein@google.com
Committed: http://code.google.com/p/skia/source/detail?r=12996
Patch Set 1 #Patch Set 2 : ComputeChecksum #Patch Set 3 : GetTestDataChecksum #Patch Set 4 : TestChecksumSelfConsistency #Patch Set 5 : DEF_TEST - Checksum #Patch Set 6 : rm ChecksumTestClass #Patch Set 7 : counterproposal #
Total comments: 22
Patch Set 8 : bungeman #
Total comments: 2
Patch Set 9 : align4 #Messages
Total messages: 18 (0 generated)
your pleasure. ;)
On 2014/01/08 00:57:39, tfarina wrote: > your pleasure. ;) This test looked so gross I just had to rewrite it. I've uploaded a counterproposal that tests all the functionality the old test did except those particular known collisions for SkChecksum::Compute, which don't seem particularly important to test now that we have SkChecksum::Murmur3 as an option. What do you think?
On 2014/01/08 15:47:24, mtklein wrote: > On 2014/01/08 00:57:39, tfarina wrote: > > your pleasure. ;) > > This test looked so gross I just had to rewrite it. I've uploaded a > counterproposal that tests all the functionality the old test did except those > particular known collisions for SkChecksum::Compute, which don't seem > particularly important to test now that we have SkChecksum::Murmur3 as an > option. What do you think? I don't see it, did you forget to upload it?
On 2014/01/08 16:46:53, tfarina wrote: > On 2014/01/08 15:47:24, mtklein wrote: > > On 2014/01/08 00:57:39, tfarina wrote: > > > your pleasure. ;) > > > > This test looked so gross I just had to rewrite it. I've uploaded a > > counterproposal that tests all the functionality the old test did except those > > particular known collisions for SkChecksum::Compute, which don't seem > > particularly important to test now that we have SkChecksum::Murmur3 as an > > option. What do you think? > > I don't see it, did you forget to upload it? PS 7?
Mike, I did my analysis below, as I was reading the code, I was commenting it to make sure I understand it. Please, see if I understood it right. You may get Cary Clark or Mike Reed to do a real review for you though. https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp File tests/ChecksumTest.cpp (right): https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:20: uint32_t(*kAlgorithms[])(const uint32_t*, size_t) = { array of pointers to functions, the beauty of C :) https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:29: for (size_t i = 0; i < SK_ARRAY_COUNT(tweaked); i++) data[i] = tweaked[i] = rand.nextU(); ok, you put random numbers in an array of 32 elements. https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:32: for (size_t i = 0; i < SK_ARRAY_COUNT(kAlgorithms); i++) { then you iterate through the array of function pointers, to test SkChecksum::Compute and SkChecksum::Murmur3 https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:36: const uint32_t hash = kAlgorithms[i](data, kBytes); so you call the algorithm to hash the data and store it to... https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:38: ASSERT(hash == kAlgorithms[i](data, kBytes)); make sure it will always return the same hash for the same data you pass. https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:40: // Changing any one element should change the hash. "any one element", that is hard to translate to my native language, Portuguese. could you find a better word? https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:43: tweaked[j] = rand.nextU(); ok, so here you are changing the data you have previous stored in the original array, hence you named the array 'tweaked'. https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:44: const uint32_t tweakedHash = kAlgorithms[i](tweaked, kBytes); then you call the algorithm again, to obtain a new hash. https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:45: ASSERT(tweakedHash != hash); and it should be different for the original hash, because you changed the value of the elements of the array 'tweaked'. https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:46: ASSERT(tweakedHash == kAlgorithms[i](tweaked, kBytes)); then you hash again to make sure the algorithm is deterministic. https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:47: tweaked[j] = saved; and restore the original value (that part I didn't follow, is it necessary?).
I think you understood it correctly. https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp File tests/ChecksumTest.cpp (right): https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:20: uint32_t(*kAlgorithms[])(const uint32_t*, size_t) = { On 2014/01/08 18:32:40, tfarina wrote: > array of pointers to functions, the beauty of C :) Yeah, this always takes me forever to remember how to even type it in. https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:40: // Changing any one element should change the hash. On 2014/01/08 18:32:40, tfarina wrote: > "any one element", that is hard to translate to my native language, Portuguese. > could you find a better word? "any single element"? The idea is that we have [ n0 n1 n2 n3 ... n31 ] in our array. We want to make sure that our hash function is sensitive enough to changes that changing any of those 32 n's to a different value, even just one, changes the hash. This is a simple approximation of a full avalanche test, http://pt.wikipedia.org/wiki/Efeito_avalanche_(criptografia) . https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:47: tweaked[j] = saved; On 2014/01/08 18:32:40, tfarina wrote: > and restore the original value (that part I didn't follow, is it necessary?). Not strictly necessary, but we want to make sure we're only looking at one changed number at a time. This loop checks each location independently. If we left out tweaked[j] = saved;, we'd be testing j = 0: is the hash different if we change one number? j = 1: is the hash different if we change two numbers? j = 2: is the hash different if we change three numbers? etc.
https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp File tests/ChecksumTest.cpp (right): https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:20: uint32_t(*kAlgorithms[])(const uint32_t*, size_t) = { I know that doing this inline is awesome and all, but maybe make this a typedef? typedef uint32_t (*algorithmProc)(const uint32_t*, size_t); algorithmProc kAlgorithms[] = { ... https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:29: for (size_t i = 0; i < SK_ARRAY_COUNT(tweaked); i++) data[i] = tweaked[i] = rand.nextU(); The magic of putting this all on one line is almost too much to resist. But I think this is a bit easier to read like for (...) { ...body... } or maybe like for (size_t i = 0; i < SK_ARRAY_COUNT(tweaked); data[i] = tweaked[i] = rand.nextU(), ++i); haha, I kid. https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:32: for (size_t i = 0; i < SK_ARRAY_COUNT(kAlgorithms); i++) { Totally ocd, but the post increment makes me wince. I like ++i for all those reasons that don't matter here. https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:34: ASSERT(kAlgorithms[i](NULL, 0) == 0); There's a lot of kAlgorithms[i] going on here, can we get a algorithmProc& currentAlgorithm = kAlgorithms[i]; and use that instead? Makes things a bit clearer.
All done. https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp File tests/ChecksumTest.cpp (right): https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:20: uint32_t(*kAlgorithms[])(const uint32_t*, size_t) = { On 2014/01/09 16:08:33, bungeman1 wrote: > I know that doing this inline is awesome and all, but maybe make this a typedef? > > typedef uint32_t (*algorithmProc)(const uint32_t*, size_t); > algorithmProc kAlgorithms[] = { ... Done. https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:29: for (size_t i = 0; i < SK_ARRAY_COUNT(tweaked); i++) data[i] = tweaked[i] = rand.nextU(); On 2014/01/09 16:08:33, bungeman1 wrote: > The magic of putting this all on one line is almost too much to resist. But I > think this is a bit easier to read like > > for (...) { > ...body... > } > > or maybe like > > for (size_t i = 0; i < SK_ARRAY_COUNT(tweaked); data[i] = tweaked[i] = > rand.nextU(), ++i); > > haha, I kid. Done. https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:32: for (size_t i = 0; i < SK_ARRAY_COUNT(kAlgorithms); i++) { On 2014/01/09 16:08:33, bungeman1 wrote: > Totally ocd, but the post increment makes me wince. I like ++i for all those > reasons that don't matter here. Done. https://codereview.chromium.org/126743003/diff/130001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:34: ASSERT(kAlgorithms[i](NULL, 0) == 0); On 2014/01/09 16:08:33, bungeman1 wrote: > There's a lot of kAlgorithms[i] going on here, can we get a > > algorithmProc& currentAlgorithm = kAlgorithms[i]; > > and use that instead? Makes things a bit clearer. > Done.
PS8 lgtm with one silly request which may be ignored at the author's discretion. https://codereview.chromium.org/126743003/diff/230001/tests/ChecksumTest.cpp File tests/ChecksumTest.cpp (right): https://codereview.chromium.org/126743003/diff/230001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:27: uint32_t data[kBytes/4], tweaked[kBytes/4]; Totally not really needed, but you were just making me assert that a constant that needed to be a power of 2 actually was, so maybe we should SK_COMPILE_ASSERT here that kBytes is divisible by 4?
lgtm https://codereview.chromium.org/126743003/diff/230001/tests/ChecksumTest.cpp File tests/ChecksumTest.cpp (right): https://codereview.chromium.org/126743003/diff/230001/tests/ChecksumTest.cpp#... tests/ChecksumTest.cpp:27: uint32_t data[kBytes/4], tweaked[kBytes/4]; On 2014/01/09 17:08:20, bungeman1 wrote: > Totally not really needed, but you were just making me assert that a constant > that needed to be a power of 2 actually was, so maybe we should > SK_COMPILE_ASSERT here that kBytes is divisible by 4? Since this is sort of silly, I have solved it in a silly way, by wrapping SkAlign4 around 128.
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/tfarina@chromium.org/126743003/280001
Mike, do you want to dcommit this (since you authored this)? Otherwise, I will just do it when I get back home later tonight. CQ is evil, because it steals the credit and gives it to commit-bot@chromium.org.
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/tfarina@chromium.org/126743003/280001
Message was sent while issue was closed.
Change committed as 12996
Message was sent while issue was closed.
On 2014/01/09 17:31:46, tfarina wrote: > Mike, do you want to dcommit this (since you authored this)? Otherwise, I will > just do it when I get back home later tonight. CQ is evil, because it steals the > credit and gives it to mailto:commit-bot@chromium.org. I pretty much always use the commit queue. I'd prefer it if everyone did, but that's another issue.
Message was sent while issue was closed.
On 2014/01/09 17:50:19, mtklein wrote: > On 2014/01/09 17:31:46, tfarina wrote: > > Mike, do you want to dcommit this (since you authored this)? Otherwise, I will > > just do it when I get back home later tonight. CQ is evil, because it steals > the > > credit and gives it to mailto:commit-bot@chromium.org. > > I pretty much always use the commit queue. I'd prefer it if everyone did, but > that's another issue. Ouch, that makes me very sad. :( I do use CQ on chromium side because there they have hooks that rewrite the log to set the committer and author to the proper email address. Due to limitations in code.google.com/, that is not possible on skia until it switches to git.
Message was sent while issue was closed.
On 2014/01/09 18:02:42, tfarina wrote: > On 2014/01/09 17:50:19, mtklein wrote: > > On 2014/01/09 17:31:46, tfarina wrote: > > > Mike, do you want to dcommit this (since you authored this)? Otherwise, I > will > > > just do it when I get back home later tonight. CQ is evil, because it steals > > the > > > credit and gives it to mailto:commit-bot@chromium.org. > > > > I pretty much always use the commit queue. I'd prefer it if everyone did, but > > that's another issue. > > Ouch, that makes me very sad. :( > > I do use CQ on chromium side because there they have hooks that rewrite the log > to set the committer and author to the proper email address. Due to limitations > in code.google.com/, that is not possible on skia until it switches to git. Personally, this has never bothered me, but I'm sure we'd love the patch to fix Skia when possible. For what it's worth, the author and committer are tracked separately. This CL shows up on our build as tfarina@chromium.org (commit-bot), and the commit itself has an Author: line. |
