You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/09/25 02:40:15 UTC
[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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
[kudu-CR] cache: fix behavior on single-CPU systems
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[kudu-CR] cache: fix behavior on single-CPU systems
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
[kudu-CR] cache: fix behavior on single-CPU systems
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
---
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>