impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Date Wed, 01 Nov 2017 22:39:52 GMT
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<DiskIoRequestContext> 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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <twang@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Nov 2017 22:39:52 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message