You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Zoltan Martonka (Code Review)" <ge...@cloudera.org> on 2023/11/28 22:23:32 UTC

[kudu-CR] Fix CountOnes() function

Zoltan Martonka has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20736


Change subject: Fix CountOnes() function
......................................................................

Fix CountOnes() function

This function is only used on arms. It does not work on numbers
greather than 255:
For some reason ULL sufix was added to 2 masks, that should be
32bit, and losing the higher bits are actually the desired
behaviour.

Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc
---
M src/kudu/gutil/bits.h
1 file changed, 2 insertions(+), 2 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/20736/1
-- 
To view, visit http://gerrit.cloudera.org:8080/20736
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc
Gerrit-Change-Number: 20736
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Martonka <zm...@cloudera.com>

[kudu-CR] [ARM] Fix CountOnes() function

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20736 )

Change subject: [ARM] Fix CountOnes() function
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG@9
PS2, Line 9: arms
> nit: ARM
Missed addressing this one?



-- 
To view, visit http://gerrit.cloudera.org:8080/20736
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc
Gerrit-Change-Number: 20736
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Dec 2023 18:11:52 +0000
Gerrit-HasComments: Yes

[kudu-CR] [arm64] Fix CountOnes() function

Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20736 )

Change subject: [arm64] Fix CountOnes() function
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG@7
PS2, Line 7: Fix CountOnes() function
Out of curiosity - did this issue manifest into some failure?


http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG@10
PS2, Line 10: greather
nit: greater


http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG@11
PS2, Line 11: sufix
nit: suffix



-- 
To view, visit http://gerrit.cloudera.org:8080/20736
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc
Gerrit-Change-Number: 20736
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Nov 2023 16:10:19 +0000
Gerrit-HasComments: Yes

[kudu-CR] [ARM] Fix CountOnes() function

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20736 )

Change subject: [ARM] Fix CountOnes() function
......................................................................

[ARM] Fix CountOnes() function

This function is only used on arms. It does not work on numbers
greater than 255:
For some reason ULL suffix was added to 2 masks, that should be
32bit, and losing the higher bits are actually the desired
behaviour.

Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc
Reviewed-on: http://gerrit.cloudera.org:8080/20736
Reviewed-by: Alexey Serbin <al...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/gutil/bits.h
1 file changed, 6 insertions(+), 4 deletions(-)

Approvals:
  Alexey Serbin: Looks good to me, approved
  Kudu Jenkins: Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/20736
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc
Gerrit-Change-Number: 20736
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>

[kudu-CR] [ARM] Fix CountOnes() function

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20736 )

Change subject: [ARM] Fix CountOnes() function
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG@9
PS2, Line 9: arms
> Missed addressing this one?
OK, it's just a nit anyway.



-- 
To view, visit http://gerrit.cloudera.org:8080/20736
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc
Gerrit-Change-Number: 20736
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Dec 2023 17:55:52 +0000
Gerrit-HasComments: Yes

[kudu-CR] [arm64] Fix CountOnes() function

Posted by "Zoltan Martonka (Code Review)" <ge...@cloudera.org>.
Zoltan Martonka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20736 )

Change subject: [arm64] Fix CountOnes() function
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG@7
PS2, Line 7: Fix CountOnes() function
> Out of curiosity - did this issue manifest into some failure?
bitmap-test TestBitMap.TestIteration was failing.
inline void ForEachBit(...) ub bitmap.h has 
int tot_count = Bits::CountOnes64withPopcount(w); in line 216.
This returned 33686278 instead of 6. It also caused the test to fail really slow, because it prints this array (filled with "64") to std::cout.  

This function  (ForEachSetBit) is also used in rowlock.cc, but perf test runs fine, I don't know what exactly uses the corresponding function.



-- 
To view, visit http://gerrit.cloudera.org:8080/20736
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc
Gerrit-Change-Number: 20736
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 Nov 2023 09:28:21 +0000
Gerrit-HasComments: Yes

[kudu-CR] [ARM] Fix CountOnes() function

Posted by "Zoltan Martonka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Ashwani Raina, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/20736

to look at the new patch set (#3).

Change subject: [ARM] Fix CountOnes() function
......................................................................

[ARM] Fix CountOnes() function

This function is only used on arms. It does not work on numbers
greater than 255:
For some reason ULL suffix was added to 2 masks, that should be
32bit, and losing the higher bits are actually the desired
behaviour.

Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc
---
M src/kudu/gutil/bits.h
1 file changed, 4 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/20736/3
-- 
To view, visit http://gerrit.cloudera.org:8080/20736
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc
Gerrit-Change-Number: 20736
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>

[kudu-CR] [arm64] Fix CountOnes() function

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20736 )

Change subject: [arm64] Fix CountOnes() function
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG@9
PS2, Line 9: arms
nit: ARM


http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG@11
PS2, Line 11: For some reason ULL sufix was added to 2 masks,
That was my mistake -- I was trying to address ASAN warnings when running tests on aarch64.  I thought if no tests are broken, then it still works as expected, but I was wrong.

Thank you for the fix!


http://gerrit.cloudera.org:8080/#/c/20736/2/src/kudu/gutil/bits.h
File src/kudu/gutil/bits.h:

http://gerrit.cloudera.org:8080/#/c/20736/2/src/kudu/gutil/bits.h@16
PS2, Line 16: static int CountOnes(uint32 n) {
To quell UBSAN warnings, will it be necessary to add ATTRIBUTE_NO_SANITIZE_INTEGER for this function?



-- 
To view, visit http://gerrit.cloudera.org:8080/20736
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc
Gerrit-Change-Number: 20736
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Nov 2023 01:03:07 +0000
Gerrit-HasComments: Yes

[kudu-CR] Fix CountOnes() function

Posted by "Zoltan Martonka (Code Review)" <ge...@cloudera.org>.
Zoltan Martonka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20736 )

Change subject: Fix CountOnes() function
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20736/1/src/kudu/gutil/bits.h
File src/kudu/gutil/bits.h:

http://gerrit.cloudera.org:8080/#/c/20736/1/src/kudu/gutil/bits.h@19
PS1, Line 19:     return static_cast<int>((((n + (n >> 4)) & 0x0F0F0F0F) * 0x01010101) >> 24);
I tested it for every possible uint32 value against:
static int CountOnesBasic(unsigned int n) {
    int res = 0;
    while(n) {
        res += n % 2; n /=2;
    }
    return res;
}
Now it works.



-- 
To view, visit http://gerrit.cloudera.org:8080/20736
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc
Gerrit-Change-Number: 20736
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Nov 2023 22:27:33 +0000
Gerrit-HasComments: Yes

[kudu-CR] [ARM] Fix CountOnes() function

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20736 )

Change subject: [ARM] Fix CountOnes() function
......................................................................


Patch Set 4: Code-Review+2

Thank you for the fix!


-- 
To view, visit http://gerrit.cloudera.org:8080/20736
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc
Gerrit-Change-Number: 20736
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Dec 2023 18:09:55 +0000
Gerrit-HasComments: No

[kudu-CR] [ARM] Fix CountOnes() function

Posted by "Zoltan Martonka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Ashwani Raina, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/20736

to look at the new patch set (#4).

Change subject: [ARM] Fix CountOnes() function
......................................................................

[ARM] Fix CountOnes() function

This function is only used on arms. It does not work on numbers
greater than 255:
For some reason ULL suffix was added to 2 masks, that should be
32bit, and losing the higher bits are actually the desired
behaviour.

Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc
---
M src/kudu/gutil/bits.h
1 file changed, 6 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/20736/4
-- 
To view, visit http://gerrit.cloudera.org:8080/20736
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc
Gerrit-Change-Number: 20736
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>

[kudu-CR] [arm64] Fix CountOnes() function

Posted by "Zoltan Martonka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/20736

to look at the new patch set (#2).

Change subject: [arm64] Fix CountOnes() function
......................................................................

[arm64] Fix CountOnes() function

This function is only used on arms. It does not work on numbers
greather than 255:
For some reason ULL sufix was added to 2 masks, that should be
32bit, and losing the higher bits are actually the desired
behaviour.

Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc
---
M src/kudu/gutil/bits.h
1 file changed, 2 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/20736/2
-- 
To view, visit http://gerrit.cloudera.org:8080/20736
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc
Gerrit-Change-Number: 20736
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>

[kudu-CR] [ARM] Fix CountOnes() function

Posted by "Zoltan Martonka (Code Review)" <ge...@cloudera.org>.
Zoltan Martonka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20736 )

Change subject: [ARM] Fix CountOnes() function
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG@9
PS2, Line 9: arms
> OK, it's just a nit anyway.
Sorry, I have no idea how I managed to miss it.



-- 
To view, visit http://gerrit.cloudera.org:8080/20736
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc
Gerrit-Change-Number: 20736
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Dec 2023 09:56:57 +0000
Gerrit-HasComments: Yes