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 2017/09/15 03:08:02 UTC

[kudu-CR] Avoid unnecessary vector allocations for ReadV/WriteV-like APIs 

Todd Lipcon has uploaded a new change for review.

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

Change subject: Avoid unnecessary vector<Slice> allocations for ReadV/WriteV-like APIs
......................................................................

Avoid unnecessary vector<Slice> allocations for ReadV/WriteV-like APIs

This reduces extra short-lived allocations for the various
scatter-gather (aka "vectored") read/write APIs.

These APIs used to all take vector<Slice>, which forces the caller to
allocate on the heap. Almost all of the call sites only need a small
constant number of slices, so the heap allocation is unnecessary
overhead.

This patch imports ArrayView<T> from Chromium/WebRTC. This is a view
into an existing array or vector (similar to how a StringPiece is a view
into a string or character array). The vectored read/write APIs are
converted to use ArrayView<Slice> instead of vector<Slice>.

Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/consensus/log_util.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs-test-util.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
A src/kudu/util/array_view.h
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
28 files changed, 283 insertions(+), 146 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>

[kudu-CR] Avoid unnecessary vector allocations for ReadV/WriteV-like APIs

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: Avoid unnecessary vector<Slice> allocations for ReadV/WriteV-like APIs
......................................................................


Patch Set 3:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/8077/3/src/kudu/cfile/cfile_reader.cc
File src/kudu/cfile/cfile_reader.cc:

Line 238:     std::array<const Slice, 2> slices {{ mal, header }};
> Why do you need to use std::array in some cases and a regular array in othe
at first I was excited about being C++y, but later decided that C arrays were easier to read anyway. Will switch to C arrays throughout.


PS3, Line 475:   if (has_checksums() && FLAGS_cfile_verify_checksums) {
             :     results = ArrayView<Slice>(results_backing.data(), 2);
             :   } else {
             :     results = ArrayView<Slice>(results_backing.data(), 1);
             :   }
> Would a ternary simplify this?
Done


http://gerrit.cloudera.org:8080/#/c/8077/3/src/kudu/util/array_view.h
File src/kudu/util/array_view.h:

Line 2:  *  Copyright 2015 The WebRTC Project Authors. All rights reserved.
> Need to update our LICENSE file?
done. I also filed https://issues.apache.org/jira/browse/LEGAL-330 to double-check that the PATENTS file is not a problem.


Line 17: // Many functions read from or write to arrays. The obvious way to do this is
> Would be useful to include a comment on the provenance of ArrayView, and pe
done (in the file header)


Line 57: //   Contains17(kudu::ArrayView<int>(arr, size));  // pointer + size
> Nit: indentation on the inline comment.
Done


Line 77:   template <typename U>
> For containers like these, why do the constructors wind up templatized on a
right, so long as the pointer types are implicitly convertible. eg if you have ArrayView<FooBase*> and you have a FooDerivedClass*, then that up-cast is implicitly convertible and so this constructor will be used. Same with converting from T* to const T*, etc.


PS3, Line 85: U (&array)[N]
> Can't say I've seen this syntax before. It means "array of N elements of ty
I think it's a "a reference to an array of N elements of type U". This is just copied code.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Avoid unnecessary vector allocations for ReadV/WriteV-like APIs

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.

Change subject: Avoid unnecessary vector<Slice> allocations for ReadV/WriteV-like APIs
......................................................................


Avoid unnecessary vector<Slice> allocations for ReadV/WriteV-like APIs

This reduces extra short-lived allocations for the various
scatter-gather (aka "vectored") read/write APIs.

These APIs used to all take vector<Slice>, which forces the caller to
allocate on the heap. Almost all of the call sites only need a small
constant number of slices, so the heap allocation is unnecessary
overhead.

This patch imports ArrayView<T> from Chromium/WebRTC. This is a view
into an existing array or vector (similar to how a StringPiece is a view
into a string or character array). The vectored read/write APIs are
converted to use ArrayView<Slice> instead of vector<Slice>.

Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
Reviewed-on: http://gerrit.cloudera.org:8080/8077
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M LICENSE.txt
M build-support/iwyu/iwyu-filter.awk
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/consensus/log_util.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs-test-util.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
A src/kudu/util/array_view.h
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
30 files changed, 348 insertions(+), 148 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
Gerrit-PatchSet: 5
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: Todd Lipcon <to...@apache.org>

[kudu-CR] Avoid unnecessary vector allocations for ReadV/WriteV-like APIs

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Avoid unnecessary vector<Slice> allocations for ReadV/WriteV-like APIs
......................................................................


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
Gerrit-PatchSet: 4
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Avoid unnecessary vector allocations for ReadV/WriteV-like APIs

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has uploaded a new patch set (#2).

Change subject: Avoid unnecessary vector<Slice> allocations for ReadV/WriteV-like APIs
......................................................................

Avoid unnecessary vector<Slice> allocations for ReadV/WriteV-like APIs

This reduces extra short-lived allocations for the various
scatter-gather (aka "vectored") read/write APIs.

These APIs used to all take vector<Slice>, which forces the caller to
allocate on the heap. Almost all of the call sites only need a small
constant number of slices, so the heap allocation is unnecessary
overhead.

This patch imports ArrayView<T> from Chromium/WebRTC. This is a view
into an existing array or vector (similar to how a StringPiece is a view
into a string or character array). The vectored read/write APIs are
converted to use ArrayView<Slice> instead of vector<Slice>.

Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
---
M build-support/iwyu/iwyu-filter.awk
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/consensus/log_util.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs-test-util.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
A src/kudu/util/array_view.h
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
29 files changed, 284 insertions(+), 146 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>

[kudu-CR] Avoid unnecessary vector allocations for ReadV/WriteV-like APIs

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/8077

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

Change subject: Avoid unnecessary vector<Slice> allocations for ReadV/WriteV-like APIs
......................................................................

Avoid unnecessary vector<Slice> allocations for ReadV/WriteV-like APIs

This reduces extra short-lived allocations for the various
scatter-gather (aka "vectored") read/write APIs.

These APIs used to all take vector<Slice>, which forces the caller to
allocate on the heap. Almost all of the call sites only need a small
constant number of slices, so the heap allocation is unnecessary
overhead.

This patch imports ArrayView<T> from Chromium/WebRTC. This is a view
into an existing array or vector (similar to how a StringPiece is a view
into a string or character array). The vectored read/write APIs are
converted to use ArrayView<Slice> instead of vector<Slice>.

Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
---
M LICENSE.txt
M build-support/iwyu/iwyu-filter.awk
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/consensus/log_util.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs-test-util.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
A src/kudu/util/array_view.h
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
30 files changed, 348 insertions(+), 148 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
Gerrit-PatchSet: 4
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: Todd Lipcon <to...@apache.org>

[kudu-CR] Avoid unnecessary vector allocations for ReadV/WriteV-like APIs

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Avoid unnecessary vector<Slice> allocations for ReadV/WriteV-like APIs
......................................................................


Patch Set 3:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/8077/3/src/kudu/cfile/cfile_reader.cc
File src/kudu/cfile/cfile_reader.cc:

Line 238:     std::array<const Slice, 2> slices {{ mal, header }};
Why do you need to use std::array in some cases and a regular array in others?


PS3, Line 475:   if (has_checksums() && FLAGS_cfile_verify_checksums) {
             :     results = ArrayView<Slice>(results_backing.data(), 2);
             :   } else {
             :     results = ArrayView<Slice>(results_backing.data(), 1);
             :   }
Would a ternary simplify this?


http://gerrit.cloudera.org:8080/#/c/8077/3/src/kudu/util/array_view.h
File src/kudu/util/array_view.h:

Line 2:  *  Copyright 2015 The WebRTC Project Authors. All rights reserved.
Need to update our LICENSE file?


Line 17: // Many functions read from or write to arrays. The obvious way to do this is
Would be useful to include a comment on the provenance of ArrayView, and perhaps a word or two on how it was modified for Kudu (if at all).


Line 57: //   Contains17(kudu::ArrayView<int>(arr, size));  // pointer + size
Nit: indentation on the inline comment.


Line 77:   template <typename U>
For containers like these, why do the constructors wind up templatized on a different type than the class's template type? Is it because declaring it this way lets the constructor be used both when U == T and when U != T?


PS3, Line 85: U (&array)[N]
Can't say I've seen this syntax before. It means "array of N elements of type U?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
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-HasComments: Yes

[kudu-CR] Avoid unnecessary vector allocations for ReadV/WriteV-like APIs

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: Avoid unnecessary vector<Slice> allocations for ReadV/WriteV-like APIs
......................................................................


Patch Set 4:

I'm going to go ahead and push this rather than wait for the LEGAL jira to be resolved. So long as it's approved prior to our next release we should be fine, and I feel relatively confident in it. If for some reason it's not approved we can work on swapping out ArrayView for our own implementation or gsl::span

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
Gerrit-PatchSet: 4
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Avoid unnecessary vector allocations for ReadV/WriteV-like APIs

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/8077

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

Change subject: Avoid unnecessary vector<Slice> allocations for ReadV/WriteV-like APIs
......................................................................

Avoid unnecessary vector<Slice> allocations for ReadV/WriteV-like APIs

This reduces extra short-lived allocations for the various
scatter-gather (aka "vectored") read/write APIs.

These APIs used to all take vector<Slice>, which forces the caller to
allocate on the heap. Almost all of the call sites only need a small
constant number of slices, so the heap allocation is unnecessary
overhead.

This patch imports ArrayView<T> from Chromium/WebRTC. This is a view
into an existing array or vector (similar to how a StringPiece is a view
into a string or character array). The vectored read/write APIs are
converted to use ArrayView<Slice> instead of vector<Slice>.

Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
---
M build-support/iwyu/iwyu-filter.awk
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/consensus/log_util.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs-test-util.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
A src/kudu/util/array_view.h
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
29 files changed, 286 insertions(+), 146 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins