[kudu-CR] cache: fix behavior on single-CPU systems
Adar Dembo has posted comments on this change. Change subject: cache: fix behavior on single-CPU systems .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7809e5697df657a589b2ceae5c6d4edbf161b52a Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Marcel Kornacker Gerrit-HasComments: No
[kudu-CR] cache: fix behavior on single-CPU systems
Adar Dembo has submitted this change and it was merged. Change subject: cache: fix behavior on single-CPU systems .. cache: fix behavior on single-CPU systems On a system with only a single CPU, shard_bits_ would be set to 0. This would then result in calculating 'hash >> (32 - 0)' which is undefined behavior. With optimizations, this would turn into a no-op, and we'd end up using the whole hash as the shard index, instead of 0, causing a crash. The fix is to widen the hash to uint64_t before shifting. Tested by manually making NumCPUs return 1 and running cache-test. Change-Id: I7809e5697df657a589b2ceae5c6d4edbf161b52a Reviewed-on: http://gerrit.cloudera.org:8080/4535 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M src/kudu/util/cache.cc 1 file changed, 3 insertions(+), 1 deletion(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/4535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I7809e5697df657a589b2ceae5c6d4edbf161b52a Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Marcel Kornacker
[kudu-CR] cache: fix behavior on single-CPU systems
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4535 to look at the new patch set (#2). Change subject: cache: fix behavior on single-CPU systems .. cache: fix behavior on single-CPU systems On a system with only a single CPU, shard_bits_ would be set to 0. This would then result in calculating 'hash >> (32 - 0)' which is undefined behavior. With optimizations, this would turn into a no-op, and we'd end up using the whole hash as the shard index, instead of 0, causing a crash. The fix is to widen the hash to uint64_t before shifting. Tested by manually making NumCPUs return 1 and running cache-test. Change-Id: I7809e5697df657a589b2ceae5c6d4edbf161b52a --- M src/kudu/util/cache.cc 1 file changed, 3 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/35/4535/2 -- To view, visit http://gerrit.cloudera.org:8080/4535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7809e5697df657a589b2ceae5c6d4edbf161b52a Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Marcel Kornacker
[kudu-CR] cache: fix behavior on single-CPU systems
Hello Marcel Kornacker, Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/4535 to review the following change. Change subject: cache: fix behavior on single-CPU systems .. cache: fix behavior on single-CPU systems On a system with only a single CPU, shard_bits_ would be set to 0. This would then result in calculating 'hash >> (32 - 0)' which is undefined behavior. With optimizations, this would turn into a no-op, and we'd end up using the whole hash as the shard index, instead of 0, causing a crash. The fix is to use masking instead of shifting to determine the shard. This means we'll now use the low bits instead of high bits of the hash value, but we assume that the hash has good distribution and there is no particular downside to using low bits instead of high ones. Tested by manually making NumCPUs return 1 and running cache-test. Change-Id: I7809e5697df657a589b2ceae5c6d4edbf161b52a --- M src/kudu/util/cache.cc 1 file changed, 13 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/35/4535/1 -- To view, visit http://gerrit.cloudera.org:8080/4535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I7809e5697df657a589b2ceae5c6d4edbf161b52a Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Marcel Kornacker