Adar Dembo has posted comments on this change. Change subject: env: add ReadV() API ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/6779/1/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: PS1, Line 152: Slices Slice's http://gerrit.cloudera.org:8080/#/c/6779/1/src/kudu/util/env.h File src/kudu/util/env.h: PS1, Line 387: Slices Slice's PS1, Line 527: Slices Slice's http://gerrit.cloudera.org:8080/#/c/6779/1/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: Line 312: Status DoReadV(int fd, const string& filename, uint64_t offset, vector<Slice>* results) { This implementation seems to be structured in the following way: - vector<Slice> is the source of truth - In a loop: - Before the preadv(), convert vector<Slice> into iov array. - After the preadv(), update vector<Slice> using results. These vectors aren't long, but that's still a fair amount of copying and allocating. Instead, do you think it'd be possible to use the iov array as the source of truth? The number of elements in the array should only decrease with each preadv(), not increase, and you needn't redeclare it on the stack to do that; you can decrement iov_size to "remove" the last element in iov_array. So the body would look something like this: - Convert 'results' to iov array and iov_size up front. - In a loop: - Call preadv(). - Use preadv() results to change iov array entry base/bounds and iov_size. PS1, Line 328: Slice I think ref-based iteration (Slice&) is acceptable here, as you did on L357. -- To view, visit http://gerrit.cloudera.org:8080/6779 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
