You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2021/09/16 08:18:07 UTC
[kudu-CR] [ntp] fix two arithmetic operator
shenxingwuying@gmail.com has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17852
Change subject: [ntp] fix two arithmetic operator
......................................................................
[ntp] fix two arithmetic operator
use shift operator instead multiply/divided
Change-Id: I5775d9206e82dfd1febd98219dbde1bb69a44240
---
M src/kudu/clock/hybrid_clock.cc
M src/kudu/clock/system_ntp.cc
2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/17852/1
--
To view, visit http://gerrit.cloudera.org:8080/17852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5775d9206e82dfd1febd98219dbde1bb69a44240
Gerrit-Change-Number: 17852
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <sh...@gmail.com>
[kudu-CR] [ntp] improve two arithmetic operator
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/17852 )
Change subject: [ntp] improve two arithmetic operator
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17852/3/src/kudu/clock/hybrid_clock.cc
File src/kudu/clock/hybrid_clock.cc:
http://gerrit.cloudera.org:8080/#/c/17852/3/src/kudu/clock/hybrid_clock.cc@587
PS3, Line 587: poll_backoff_ms = std::min(poll_backoff_ms << 1, 1000);
No big deal, considering it's already merged, but in general I think these kinds of changes are not improvements:
- as you pointed out, every compiler out there already makes such optimizations when it's safe to do so
- in this case, it's logically supposed to be "doubling", not a bitwise pattern that we're trying to express
- this isn't a hot path, so even if it were a valid micro-optimization, it wouldn't matter.
--
To view, visit http://gerrit.cloudera.org:8080/17852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5775d9206e82dfd1febd98219dbde1bb69a44240
Gerrit-Change-Number: 17852
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 18 Oct 2021 16:46:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ntp] improve two arithmetic operator
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/17852 )
Change subject: [ntp] improve two arithmetic operator
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/17852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5775d9206e82dfd1febd98219dbde1bb69a44240
Gerrit-Change-Number: 17852
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 18 Oct 2021 09:37:55 +0000
Gerrit-HasComments: No
[kudu-CR] [ntp] fix two arithmetic operator
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/17852 )
Change subject: [ntp] fix two arithmetic operator
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17852/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17852/1//COMMIT_MSG@7
PS1, Line 7: fix
It's kind of 'improve' or 'optimize', not 'fix'
--
To view, visit http://gerrit.cloudera.org:8080/17852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5775d9206e82dfd1febd98219dbde1bb69a44240
Gerrit-Change-Number: 17852
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Fri, 17 Sep 2021 03:12:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ntp] improve two arithmetic operator
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/17852 )
Change subject: [ntp] improve two arithmetic operator
......................................................................
Patch Set 2: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/17852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5775d9206e82dfd1febd98219dbde1bb69a44240
Gerrit-Change-Number: 17852
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 18 Oct 2021 09:37:29 +0000
Gerrit-HasComments: No
[kudu-CR] [ntp] improve two arithmetic operator
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has removed a vote on this change.
Change subject: [ntp] improve two arithmetic operator
......................................................................
Removed Verified+1 by Yingchun Lai <ac...@gmail.com>
--
To view, visit http://gerrit.cloudera.org:8080/17852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I5775d9206e82dfd1febd98219dbde1bb69a44240
Gerrit-Change-Number: 17852
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] [ntp] improve two arithmetic operator
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
shenxingwuying@gmail.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/17852 )
Change subject: [ntp] improve two arithmetic operator
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17852/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17852/1//COMMIT_MSG@7
PS1, Line 7: imp
> It's kind of 'improve' or 'optimize', not 'fix'
ok, already change the message
http://gerrit.cloudera.org:8080/#/c/17852/1/src/kudu/clock/hybrid_clock.cc
File src/kudu/clock/hybrid_clock.cc:
http://gerrit.cloudera.org:8080/#/c/17852/1/src/kudu/clock/hybrid_clock.cc@587
PS1, Line 587: poll_backoff_ms << 1
> I'm not sure this brings any optimization: I guess the compiler is smart en
Yes, you are right. g++ has optimized it.
--
To view, visit http://gerrit.cloudera.org:8080/17852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5775d9206e82dfd1febd98219dbde1bb69a44240
Gerrit-Change-Number: 17852
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 27 Sep 2021 08:39:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ntp] improve two arithmetic operator
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17852 )
Change subject: [ntp] improve two arithmetic operator
......................................................................
[ntp] improve two arithmetic operator
use shift operator instead multiply/divided, compiler
maybe optimize it (gcc/g++ can do it).
Change-Id: I5775d9206e82dfd1febd98219dbde1bb69a44240
Reviewed-on: http://gerrit.cloudera.org:8080/17852
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Yingchun Lai <ac...@gmail.com>
---
M src/kudu/clock/hybrid_clock.cc
M src/kudu/clock/system_ntp.cc
2 files changed, 2 insertions(+), 2 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, but someone else must approve
Yingchun Lai: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/17852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I5775d9206e82dfd1febd98219dbde1bb69a44240
Gerrit-Change-Number: 17852
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] [ntp] improve two arithmetic operator
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yingchun Lai, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17852
to look at the new patch set (#2).
Change subject: [ntp] improve two arithmetic operator
......................................................................
[ntp] improve two arithmetic operator
use shift operator instead multiply/divided, compiler
maybe optimize it (gcc/g++ can do it).
Change-Id: I5775d9206e82dfd1febd98219dbde1bb69a44240
---
M src/kudu/clock/hybrid_clock.cc
M src/kudu/clock/system_ntp.cc
2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/17852/2
--
To view, visit http://gerrit.cloudera.org:8080/17852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5775d9206e82dfd1febd98219dbde1bb69a44240
Gerrit-Change-Number: 17852
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] [ntp] improve two arithmetic operator
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/17852 )
Change subject: [ntp] improve two arithmetic operator
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/17852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5775d9206e82dfd1febd98219dbde1bb69a44240
Gerrit-Change-Number: 17852
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 18 Oct 2021 09:38:01 +0000
Gerrit-HasComments: No
[kudu-CR] [ntp] fix two arithmetic operator
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17852 )
Change subject: [ntp] fix two arithmetic operator
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17852/1/src/kudu/clock/hybrid_clock.cc
File src/kudu/clock/hybrid_clock.cc:
http://gerrit.cloudera.org:8080/#/c/17852/1/src/kudu/clock/hybrid_clock.cc@587
PS1, Line 587: poll_backoff_ms << 1
I'm not sure this brings any optimization: I guess the compiler is smart enough to do its on its own, at least if compiling in release mode.
Did you see any change in the generated assembly after this update?
--
To view, visit http://gerrit.cloudera.org:8080/17852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5775d9206e82dfd1febd98219dbde1bb69a44240
Gerrit-Change-Number: 17852
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Fri, 17 Sep 2021 04:56:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ntp] improve two arithmetic operator
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17852 )
Change subject: [ntp] improve two arithmetic operator
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/17852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5775d9206e82dfd1febd98219dbde1bb69a44240
Gerrit-Change-Number: 17852
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 18 Oct 2021 04:21:00 +0000
Gerrit-HasComments: No
[kudu-CR] [ntp] improve two arithmetic operator
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
shenxingwuying@gmail.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/17852 )
Change subject: [ntp] improve two arithmetic operator
......................................................................
Patch Set 2:
> Patch Set 1:
>
> (1 comment)
Yes, you are right. g++ has optimized it.
--
To view, visit http://gerrit.cloudera.org:8080/17852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5775d9206e82dfd1febd98219dbde1bb69a44240
Gerrit-Change-Number: 17852
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 27 Sep 2021 08:36:29 +0000
Gerrit-HasComments: No