Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/8408 )
Change subject: IMPALA-6121: remove I/O mgr request context cache ...................................................................... Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/8408/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8408/4//COMMIT_MSG@12 PS4, Line 12: from TCMalloc's thread caches should scale much better than a Though reader_context_ is not frequently allocated in current code, theoretically for reducing allocation won't it be better to store DiskIoRequestContext directly (or boost::optional<> for a "nullable cell")? http://gerrit.cloudera.org:8080/#/c/8408/4/be/src/runtime/disk-io-mgr-reader-context.h File be/src/runtime/disk-io-mgr-reader-context.h: http://gerrit.cloudera.org:8080/#/c/8408/4/be/src/runtime/disk-io-mgr-reader-context.h@274 PS4, Line 274: __sync_synchronize(); Just a question - Does b need to be atomic here? If it does not generate a MOV then this __sync_synchronize might not be effective as well http://gerrit.cloudera.org:8080/#/c/8408/4/be/src/runtime/disk-io-mgr-reader-context.h@319 PS4, Line 319: if (!is_on_queue_ && num_threads_in_op_.Load() == 0 && !done_) { The return value of Add can be used to remove this Load http://gerrit.cloudera.org:8080/#/c/8408/4/be/src/runtime/disk-io-mgr-test.cc File be/src/runtime/disk-io-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/8408/4/be/src/runtime/disk-io-mgr-test.cc@251 PS4, Line 251: unique_ptr<DiskIoRequestContext> writer = io_mgr.RegisterContext(NULL); nullptr? -- To view, visit http://gerrit.cloudera.org:8080/8408 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I91414eceaa4938fccd74686fe6bebede6ef36108 Gerrit-Change-Number: 8408 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 01 Nov 2017 21:47:58 +0000 Gerrit-HasComments: Yes
