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 2018/02/01 23:30:47 UTC
[kudu-CR] jsonwriter: small optimization for repeated PB fields
Hello Will Berkeley,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/9184
to review the following change.
Change subject: jsonwriter: small optimization for repeated PB fields
......................................................................
jsonwriter: small optimization for repeated PB fields
This avoids repeated non-inlinable calls into the protobuf library in
the hot path of serializing repeated PB fields. These fields make up a
lot of the cost of serializing metrics.
Before:
I0201 14:27:49.924299 19742 jsonwriter-test.cc:193] Throughput: 112.182MB/sec
After:
I0201 14:57:32.208633 23334 jsonwriter-test.cc:193] Throughput: 159.09MB/sec
Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
---
M src/kudu/util/jsonwriter-test.cc
M src/kudu/util/jsonwriter.cc
M src/kudu/util/jsonwriter.h
3 files changed, 25 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/9184/1
--
To view, visit http://gerrit.cloudera.org:8080/9184
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
Gerrit-Change-Number: 9184
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] jsonwriter: small optimization for repeated PB fields
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9184 )
Change subject: jsonwriter: small optimization for repeated PB fields
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9184/1/src/kudu/util/jsonwriter.cc
File src/kudu/util/jsonwriter.cc:
http://gerrit.cloudera.org:8080/#/c/9184/1/src/kudu/util/jsonwriter.cc@197
PS1, Line 197: ProtobufField(pb, field);
Why not do the same here? I'm assuming pb.GetReflection() is non-inline-able as well?
--
To view, visit http://gerrit.cloudera.org:8080/9184
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
Gerrit-Change-Number: 9184
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 23:34:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] jsonwriter: small optimization for PB fields
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9184 )
Change subject: jsonwriter: small optimization for PB fields
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/9184
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
Gerrit-Change-Number: 9184
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sat, 03 Feb 2018 01:23:39 +0000
Gerrit-HasComments: No
[kudu-CR] jsonwriter: small optimization for PB fields
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9184 )
Change subject: jsonwriter: small optimization for PB fields
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9184
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
Gerrit-Change-Number: 9184
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sat, 03 Feb 2018 01:53:01 +0000
Gerrit-HasComments: No
[kudu-CR] jsonwriter: small optimization for PB fields
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9184
to look at the new patch set (#2).
Change subject: jsonwriter: small optimization for PB fields
......................................................................
jsonwriter: small optimization for PB fields
This avoids extra non-inlinable calls into the protobuf library:
- extra calls to GetReflection()
- calls to FieldSize() while looping through repeated fields
This has a small benefit on all PBs and a big benefit on those PBs with
a lot of repeated fields. The latter should be a nice boost particularly
when serializing metrics.
Before:
TestJsonWriter.BenchmarkAllTypes Throughput: 157.609MB/sec
TestJsonWriter.BenchmarkNestedMessage Throughput: 124.048MB/sec
TestJsonWriter.BenchmarkRepeatedInt64 Throughput: 113.77MB/sec
After:
TestJsonWriter.BenchmarkAllTypes Throughput: 163.327MB/sec (+3.6%)
TestJsonWriter.BenchmarkNestedMessage Throughput: 127.633MB/sec (+2.9%)
TestJsonWriter.BenchmarkRepeatedInt64 Throughput: 158.229MB/sec (+39.1%)
Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
---
M src/kudu/util/jsonwriter-test.cc
M src/kudu/util/jsonwriter.cc
M src/kudu/util/jsonwriter.h
3 files changed, 36 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/9184/2
--
To view, visit http://gerrit.cloudera.org:8080/9184
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
Gerrit-Change-Number: 9184
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] jsonwriter: small optimization for repeated PB fields
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9184 )
Change subject: jsonwriter: small optimization for repeated PB fields
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9184
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
Gerrit-Change-Number: 9184
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 21:12:31 +0000
Gerrit-HasComments: No
[kudu-CR] jsonwriter: small optimization for PB fields
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9184 )
Change subject: jsonwriter: small optimization for PB fields
......................................................................
jsonwriter: small optimization for PB fields
This avoids extra non-inlinable calls into the protobuf library:
- extra calls to GetReflection()
- calls to FieldSize() while looping through repeated fields
This has a small benefit on all PBs and a big benefit on those PBs with
a lot of repeated fields. The latter should be a nice boost particularly
when serializing metrics.
Before:
TestJsonWriter.BenchmarkAllTypes Throughput: 157.609MB/sec
TestJsonWriter.BenchmarkNestedMessage Throughput: 124.048MB/sec
TestJsonWriter.BenchmarkRepeatedInt64 Throughput: 113.77MB/sec
After:
TestJsonWriter.BenchmarkAllTypes Throughput: 163.327MB/sec (+3.6%)
TestJsonWriter.BenchmarkNestedMessage Throughput: 127.633MB/sec (+2.9%)
TestJsonWriter.BenchmarkRepeatedInt64 Throughput: 158.229MB/sec (+39.1%)
Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
Reviewed-on: http://gerrit.cloudera.org:8080/9184
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/util/jsonwriter-test.cc
M src/kudu/util/jsonwriter.cc
M src/kudu/util/jsonwriter.h
3 files changed, 36 insertions(+), 11 deletions(-)
Approvals:
Sailesh Mukil: Looks good to me, but someone else must approve
Kudu Jenkins: Verified
Todd Lipcon: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/9184
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
Gerrit-Change-Number: 9184
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] jsonwriter: small optimization for repeated PB fields
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9184 )
Change subject: jsonwriter: small optimization for repeated PB fields
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9184/1/src/kudu/util/jsonwriter.cc
File src/kudu/util/jsonwriter.cc:
http://gerrit.cloudera.org:8080/#/c/9184/1/src/kudu/util/jsonwriter.cc@197
PS1, Line 197: ProtobufField(pb, field);
> Why not do the same here? I'm assuming pb.GetReflection() is non-inline-abl
good point, will do.
--
To view, visit http://gerrit.cloudera.org:8080/9184
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
Gerrit-Change-Number: 9184
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sat, 03 Feb 2018 00:54:37 +0000
Gerrit-HasComments: Yes