[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Impala Public Jenkins 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 10: Verified+1 -- 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: 10 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 15 Nov 2017 22:05:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8408 ) Change subject: IMPALA-6121: remove I/O mgr request context cache .. IMPALA-6121: remove I/O mgr request context cache This simplifies the lifecycle of the request contexts and eliminates some code. The comments claim that request context cache improves performance when allocating smallish the objects. But allocating from TCMalloc's thread caches should scale much better than a global object pool protected by a lock. I needed to move the definition to a non-internal header file so that it was visible to clients that manage it by unique_ptr. We also do not need to transfer the request contexts to the RuntimeState since I/O buffers do not leave scanners now. Testing: Ran exhaustive tests. Change-Id: I91414eceaa4938fccd74686fe6bebede6ef36108 Reviewed-on: http://gerrit.cloudera.org:8080/8408 Reviewed-by: Tim Armstrong Tested-by: Impala Public Jenkins --- M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-base.h M be/src/exec/hdfs-scan-node-mt.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/disk-io-mgr-internal.h M be/src/runtime/disk-io-mgr-reader-context.cc A be/src/runtime/disk-io-mgr-reader-context.h M be/src/runtime/disk-io-mgr-stress.cc M be/src/runtime/disk-io-mgr-test.cc M be/src/runtime/disk-io-mgr.cc M be/src/runtime/disk-io-mgr.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 16 files changed, 652 insertions(+), 811 deletions(-) Approvals: Tim Armstrong: Looks good to me, approved Impala Public Jenkins: Verified -- 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: merged Gerrit-Change-Id: I91414eceaa4938fccd74686fe6bebede6ef36108 Gerrit-Change-Number: 8408 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Dan Hecht 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 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/8408/8/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/8/be/src/runtime/disk-io-mgr-reader-context.h@27 PS8, Line 27: but some clients may need to include this header, e.g. to make the : /// unique_ptr destructor work correctly. : /// > I think exposing it will make more sense after a follow-on patch. There are Yeah, but then making sure this abstraction makes sense becomes more important, which is why I was getting a little nervous about what a DiskIoRequestContext actually represents. But we can deal with that in that change. -- 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: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 15 Nov 2017 19:04:23 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Impala Public Jenkins 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 10: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1476/ -- 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: 10 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 15 Nov 2017 18:48:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Tim Armstrong 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 10: Code-Review+2 Rebased onto the ConditionVariable change -- 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: 10 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 15 Nov 2017 18:47:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Hello Michael Ho, Tianyi Wang, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8408 to look at the new patch set (#10). Change subject: IMPALA-6121: remove I/O mgr request context cache .. IMPALA-6121: remove I/O mgr request context cache This simplifies the lifecycle of the request contexts and eliminates some code. The comments claim that request context cache improves performance when allocating smallish the objects. But allocating from TCMalloc's thread caches should scale much better than a global object pool protected by a lock. I needed to move the definition to a non-internal header file so that it was visible to clients that manage it by unique_ptr. We also do not need to transfer the request contexts to the RuntimeState since I/O buffers do not leave scanners now. Testing: Ran exhaustive tests. Change-Id: I91414eceaa4938fccd74686fe6bebede6ef36108 --- M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-base.h M be/src/exec/hdfs-scan-node-mt.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/disk-io-mgr-internal.h M be/src/runtime/disk-io-mgr-reader-context.cc A be/src/runtime/disk-io-mgr-reader-context.h M be/src/runtime/disk-io-mgr-stress.cc M be/src/runtime/disk-io-mgr-test.cc M be/src/runtime/disk-io-mgr.cc M be/src/runtime/disk-io-mgr.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 16 files changed, 652 insertions(+), 811 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/8408/10 -- 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: newpatchset Gerrit-Change-Id: I91414eceaa4938fccd74686fe6bebede6ef36108 Gerrit-Change-Number: 8408 Gerrit-PatchSet: 10 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Hello Michael Ho, Tianyi Wang, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8408 to look at the new patch set (#9). Change subject: IMPALA-6121: remove I/O mgr request context cache .. IMPALA-6121: remove I/O mgr request context cache This simplifies the lifecycle of the request contexts and eliminates some code. The comments claim that request context cache improves performance when allocating smallish the objects. But allocating from TCMalloc's thread caches should scale much better than a global object pool protected by a lock. I needed to move the definition to a non-internal header file so that it was visible to clients that manage it by unique_ptr. We also do not need to transfer the request contexts to the RuntimeState since I/O buffers do not leave scanners now. Testing: Ran exhaustive tests. Change-Id: I91414eceaa4938fccd74686fe6bebede6ef36108 --- M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-base.h M be/src/exec/hdfs-scan-node-mt.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/disk-io-mgr-internal.h M be/src/runtime/disk-io-mgr-reader-context.cc A be/src/runtime/disk-io-mgr-reader-context.h M be/src/runtime/disk-io-mgr-stress.cc M be/src/runtime/disk-io-mgr-test.cc M be/src/runtime/disk-io-mgr.cc M be/src/runtime/disk-io-mgr.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 16 files changed, 651 insertions(+), 811 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/8408/9 -- 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: newpatchset Gerrit-Change-Id: I91414eceaa4938fccd74686fe6bebede6ef36108 Gerrit-Change-Number: 8408 Gerrit-PatchSet: 9 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Tim Armstrong 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 8: (3 comments) http://gerrit.cloudera.org:8080/#/c/8408/8/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/8/be/src/runtime/disk-io-mgr-reader-context.h@27 PS8, Line 27: but some clients may need to include this header, e.g. to make the : /// unique_ptr destructor work correctly. : /// > that's really unfortunate, but the change is a net win. I think exposing it will make more sense after a follow-on patch. There are a number of methods in DiskIoMgr that are just indirections to RequestContext methods and I think just letting clients call them directly makes more sense. http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr.h File be/src/runtime/disk-io-mgr.h: http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr.h@654 PS7, Line 654: std::unique_ptr RegisterContext(MemTracker* reader_mem_tracker); > I wasn't thinking in terms of implementation. I was thinking in terms of th Ah ok I understand now http://gerrit.cloudera.org:8080/#/c/8408/8/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: http://gerrit.cloudera.org:8080/#/c/8408/8/be/src/runtime/runtime-state.h@39 PS8, Line 39: class DiskIoRequestContext; > delete Done -- 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: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 15 Nov 2017 18:27:50 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Dan Hecht 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 8: Code-Review+2 (3 comments) http://gerrit.cloudera.org:8080/#/c/8408/8/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/8/be/src/runtime/disk-io-mgr-reader-context.h@27 PS8, Line 27: but some clients may need to include this header, e.g. to make the : /// unique_ptr destructor work correctly. : /// that's really unfortunate, but the change is a net win. http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr.h File be/src/runtime/disk-io-mgr.h: http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr.h@654 PS7, Line 654: std::unique_ptr RegisterContext(MemTracker* reader_mem_tracker); > I think it's an implementation detail the RegisterContext() doesn't registe I wasn't thinking in terms of implementation. I was thinking in terms of the interface -- how can a context be "registered" when the caller doesn't pass a context to be registered? Anyway, I'm fine with leaving the terminology alone in this change. http://gerrit.cloudera.org:8080/#/c/8408/8/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: http://gerrit.cloudera.org:8080/#/c/8408/8/be/src/runtime/runtime-state.h@39 PS8, Line 39: class DiskIoRequestContext; delete -- 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: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 13 Nov 2017 22:05:47 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Hello Michael Ho, Tianyi Wang, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8408 to look at the new patch set (#8). Change subject: IMPALA-6121: remove I/O mgr request context cache .. IMPALA-6121: remove I/O mgr request context cache This simplifies the lifecycle of the request contexts and eliminates some code. The comments claim that request context cache improves performance when allocating smallish the objects. But allocating from TCMalloc's thread caches should scale much better than a global object pool protected by a lock. I needed to move the definition to a non-internal header file so that it was visible to clients that manage it by unique_ptr. We also do not need to transfer the request contexts to the RuntimeState since I/O buffers do not leave scanners now. Testing: Ran exhaustive tests. Change-Id: I91414eceaa4938fccd74686fe6bebede6ef36108 --- M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-base.h M be/src/exec/hdfs-scan-node-mt.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/disk-io-mgr-internal.h M be/src/runtime/disk-io-mgr-reader-context.cc A be/src/runtime/disk-io-mgr-reader-context.h M be/src/runtime/disk-io-mgr-stress.cc M be/src/runtime/disk-io-mgr-test.cc M be/src/runtime/disk-io-mgr.cc M be/src/runtime/disk-io-mgr.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 16 files changed, 651 insertions(+), 810 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/8408/8 -- 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: newpatchset Gerrit-Change-Id: I91414eceaa4938fccd74686fe6bebede6ef36108 Gerrit-Change-Number: 8408 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Tim Armstrong 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 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/8408/7/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/7/be/src/runtime/disk-io-mgr-reader-context.h@25 PS7, Line 25: /// Internal per request-context state. This object maintains a lot of state that is > so no one should include this file except for disk io mgr, is that right? i Added a comment explaining that. Some clients need to include it to make the unique_ptr destructor work (that destructor has a static assert that sizeof(DiskIoRequestContext) > 0). http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr-reader-context.cc File be/src/runtime/disk-io-mgr-reader-context.cc: http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr-reader-context.cc@108 PS7, Line 108: state_ = Inactive; > why is it that Cancel() has to do so much more work than this, when this su This function calls Cancel() on line 95. http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr.h File be/src/runtime/disk-io-mgr.h: http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr.h@654 PS7, Line 654: std::unique_ptr RegisterContext(MemTracker* reader_mem_tracker); > I'm not really sure what it means to "register" a context. Should this just I think it's an implementation detail the RegisterContext() doesn't register it in any data structures. UnregisterContext() *does* have to track down and remove references from I/O manager internal data structures. http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr.h@662 PS7, Line 662: void UnregisterContext(DiskIoRequestContext* context); > Yeah, except that the statement "context cannot be used after this call" is "unregister" to me means removing all references to it from the I/O manager. I think the name conveys that the I/O manager may hold references to it. Reworked the comment a bit. I can imagine adding operations to DiskIoRequestContext that would be reasonable to call after UnregisterContext(), e.g. exposing the current state of the context. I'm fine with switching to passing in the unique_ptr, can do that in the next PS if it seems appropriate. -- 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: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 09 Nov 2017 23:53:36 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Dan Hecht 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 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/8408/7/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/7/be/src/runtime/disk-io-mgr-reader-context.h@25 PS7, Line 25: /// Internal per request-context state. This object maintains a lot of state that is so no one should include this file except for disk io mgr, is that right? i.e. this class is opaque to everyone else? I think that's less obvious now that it's not in an -internal.h header. http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr-reader-context.cc File be/src/runtime/disk-io-mgr-reader-context.cc: http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr-reader-context.cc@108 PS7, Line 108: state_ = Inactive; why is it that Cancel() has to do so much more work than this, when this supposedly does a Cancel in addition to marking it inactive? Is this really a different kind of cancel? http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr.h File be/src/runtime/disk-io-mgr.h: http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr.h@654 PS7, Line 654: std::unique_ptr RegisterContext(MemTracker* reader_mem_tracker); I'm not really sure what it means to "register" a context. Should this just be Create()? (And I guess we have these next three methods to keep the context opaque outside of disk-io-mgr, is that right?) http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr.h@662 PS7, Line 662: void UnregisterContext(DiskIoRequestContext* context); > I don't feel strongly. I think your argument has merit but destroying the d Yeah, except that the statement "context cannot be used after this call" is what contradicts the control structure lifetime being tied to QueryState. I'm fine with leaving it around. But, what does it mean to "unregister" the context? The whole register/unregister terminology seems confusing. Does the lifecycle of these things mirror the lifecycle that we're moving toward and does it make sense to use consistent terminology? -- 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: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 09 Nov 2017 18:58:59 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Tim Armstrong 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 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr-reader-context.h File be/src/runtime/disk-io-mgr-reader-context.h: PS7: > I think you can use "git mv" to produce better diff in code review. Unfortunately git mv doesn't add any metadata - the rename detection is still based on file similarity heuristics (not sure why that didn't work in this case). -- 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: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 09 Nov 2017 01:25:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
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 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr-reader-context.h File be/src/runtime/disk-io-mgr-reader-context.h: PS7: > There was some minor cleanup - initialising member variables inline, adding I think you can use "git mv" to produce better diff in code review. -- 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: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 09 Nov 2017 00:33:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Tim Armstrong 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 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr-reader-context.h File be/src/runtime/disk-io-mgr-reader-context.h: PS7: > I assume this was a wholesale move of the class without modifications, exce There was some minor cleanup - initialising member variables inline, adding a DCHECK in the destructor, etc. I created a diff here to see how the class definition changed: https://gist.github.com/timarmstrong/f640dd200260556e1167d8ca69a67f51/revisions?diff=split http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr.h File be/src/runtime/disk-io-mgr.h: http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr.h@662 PS7, Line 662: void UnregisterContext(DiskIoRequestContext* context); > Given that the context cannot be used after this call, should we move the u I don't feel strongly. I think your argument has merit but destroying the data structure goes (somewhat) against the idea that we shouldn't tear down control structures until the very end of the query. -- 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: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 07 Nov 2017 21:10:16 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Dan Hecht 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 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr-reader-context.h File be/src/runtime/disk-io-mgr-reader-context.h: PS7: I assume this was a wholesale move of the class without modifications, except for the new methods defined in the .cc file? http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr.h File be/src/runtime/disk-io-mgr.h: http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr.h@662 PS7, Line 662: void UnregisterContext(DiskIoRequestContext* context); Given that the context cannot be used after this call, should we move the unique_ptr into this call and delete it then? -- 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: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 07 Nov 2017 18:56:26 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Tim Armstrong 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 7: Code-Review+1 rebased -- 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: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 06 Nov 2017 17:32:07 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Tim Armstrong 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 6: Code-Review+1 Rebased since some of the patches under it changed. -- 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: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 03 Nov 2017 22:30:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
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 5: Code-Review+1 -- 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: 5 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 02 Nov 2017 00:45:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Tim Armstrong 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)
Thanks for looking at the patch and for the good questions.
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, theoret
Yeah I think that's a reasonable option in a lot of cases, particularly if perf
is a concern.
Allocating on the heap can avoid some slightly tricky issues around memory
alignment (if the class requires more than the default alignment, e.g. cache
line alignment, then any class including it must also be cache line aligned).
Including everything inline also can also force viral header includes, since
any class that needs to know the layout of Y that includes X inline then also
needs to know the layout of X.
I did actually just realise that we don't have copy constructors disable for
request context, so I added that.
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
Yeah I think it would make more sense to use an explicit atomic for
is_on_queue_. The whole concurrency story here could probably be simplified a
lot but it would need some thought.
I don't want to make any subtle changes to this logic as part of a refactoring
patch, but it would be nice to simplify this code at some point.
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
I'd have to think about this carefully to understand if the Acquire barrier in
Load() makes a difference. I think you're probably right but I don't want to
potentially include subtle behavioural changes in this patch.
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 writer =
io_mgr.RegisterContext(NULL);
> nullptr?
Did it for the whole file.
--
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
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tianyi Wang
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Wed, 01 Nov 2017 22:39:52 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Hello Michael Ho, Tianyi Wang, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8408 to look at the new patch set (#5). Change subject: IMPALA-6121: remove I/O mgr request context cache .. IMPALA-6121: remove I/O mgr request context cache This simplifies the lifecycle of the request contexts and eliminates some code. The comments claim that request context cache improves performance when allocating smallish the objects. But allocating from TCMalloc's thread caches should scale much better than a global object pool protected by a lock. I needed to move the definition to a non-internal header file so that it was visible to clients that manage it by unique_ptr. We also do not need to transfer the request contexts to the RuntimeState since I/O buffers do not leave scanners now. Testing: Ran exhaustive tests. Change-Id: I91414eceaa4938fccd74686fe6bebede6ef36108 --- M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-base.h M be/src/exec/hdfs-scan-node-mt.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/disk-io-mgr-internal.h M be/src/runtime/disk-io-mgr-reader-context.cc A be/src/runtime/disk-io-mgr-reader-context.h M be/src/runtime/disk-io-mgr-stress.cc M be/src/runtime/disk-io-mgr-test.cc M be/src/runtime/disk-io-mgr.cc M be/src/runtime/disk-io-mgr.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 16 files changed, 639 insertions(+), 804 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/8408/5 -- 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: newpatchset Gerrit-Change-Id: I91414eceaa4938fccd74686fe6bebede6ef36108 Gerrit-Change-Number: 8408 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
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 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
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tianyi Wang
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Wed, 01 Nov 2017 21:47:58 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Hello Michael Ho, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8408 to look at the new patch set (#4). Change subject: IMPALA-6121: remove I/O mgr request context cache .. IMPALA-6121: remove I/O mgr request context cache This simplifies the lifecycle of the request contexts and eliminates some code. The comments claim that request context cache improves performance when allocating smallish the objects. But allocating from TCMalloc's thread caches should scale much better than a global object pool protected by a lock. I needed to move the definition to a non-internal header file so that it was visible to clients that manage it by unique_ptr. We also do not need to transfer the request contexts to the RuntimeState since I/O buffers do not leave scanners now. Testing: Ran exhaustive tests. Change-Id: I91414eceaa4938fccd74686fe6bebede6ef36108 --- M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-base.h M be/src/exec/hdfs-scan-node-mt.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/disk-io-mgr-internal.h M be/src/runtime/disk-io-mgr-reader-context.cc A be/src/runtime/disk-io-mgr-reader-context.h M be/src/runtime/disk-io-mgr-stress.cc M be/src/runtime/disk-io-mgr-test.cc M be/src/runtime/disk-io-mgr.cc M be/src/runtime/disk-io-mgr.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 16 files changed, 614 insertions(+), 780 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/8408/4 -- 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: newpatchset Gerrit-Change-Id: I91414eceaa4938fccd74686fe6bebede6ef36108 Gerrit-Change-Number: 8408 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Tim Armstrong 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 3: This touches a lot of lines but is just simplification/cleanup - no expected behavioural changes. -- 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: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 31 Oct 2017 17:48:22 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Tim Armstrong 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 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/8408/3/be/src/runtime/disk-io-mgr-reader-context.h File be/src/runtime/disk-io-mgr-reader-context.h: PS3: I should rename this to disk-io-mgr-request-context.h to match the class name. -- 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: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 31 Oct 2017 17:47:34 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Tim Armstrong has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/8408 ) Change subject: IMPALA-6121: remove I/O mgr request context cache .. IMPALA-6121: remove I/O mgr request context cache This simplifies the lifecycle of the request contexts and eliminates some code. The comments claim that request context cache improves performance when allocating smallish the objects. But allocating from TCMalloc's thread caches should scale much better than a global object pool protected by a lock. I needed to move the definition to a non-internal header file so that it was visible to clients that manage it by unique_ptr. We also do not need to transfer the request contexts to the RuntimeState since I/O buffers do not leave scanners now. Testing: Ran exhaustive tests. Change-Id: I91414eceaa4938fccd74686fe6bebede6ef36108 --- M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-base.h M be/src/exec/hdfs-scan-node-mt.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/disk-io-mgr-internal.h M be/src/runtime/disk-io-mgr-reader-context.cc A be/src/runtime/disk-io-mgr-reader-context.h M be/src/runtime/disk-io-mgr-stress.cc M be/src/runtime/disk-io-mgr-test.cc M be/src/runtime/disk-io-mgr.cc M be/src/runtime/disk-io-mgr.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 16 files changed, 614 insertions(+), 780 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/8408/3 -- 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: newpatchset Gerrit-Change-Id: I91414eceaa4938fccd74686fe6bebede6ef36108 Gerrit-Change-Number: 8408 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong
[Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Tim Armstrong has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/8408 ) Change subject: IMPALA-6121: remove I/O mgr request context cache .. IMPALA-6121: remove I/O mgr request context cache This simplifies the lifecycle of the request contexts and eliminates some code. The comments claim that request context cache improves performance when allocating smallish the objects. But allocating from TCMalloc's thread caches should scale much better than a global object pool protected by a lock. I needed to move the definition to a non-internal header file so that it was visible to clients that manage it by unique_ptr. We also do not need to transfer the request contexts to the RuntimeState since I/O buffers do not leave scanners now. Testing: Ran exhaustive tests. Change-Id: I91414eceaa4938fccd74686fe6bebede6ef36108 --- M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-base.h M be/src/exec/hdfs-scan-node-mt.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/disk-io-mgr-internal.h M be/src/runtime/disk-io-mgr-reader-context.cc A be/src/runtime/disk-io-mgr-reader-context.h M be/src/runtime/disk-io-mgr-stress.cc M be/src/runtime/disk-io-mgr-test.cc M be/src/runtime/disk-io-mgr.cc M be/src/runtime/disk-io-mgr.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 16 files changed, 628 insertions(+), 780 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/8408/2 -- 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: newpatchset Gerrit-Change-Id: I91414eceaa4938fccd74686fe6bebede6ef36108 Gerrit-Change-Number: 8408 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong
