Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8363 )

Change subject: IMPALA-4456: Address scalability issues of qs_map_lock_ and 
client_request_state_map_lock_
......................................................................


Patch Set 6:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h
File be/src/util/sharded-query-map-util.h:

http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@29
PS6, Line 29: #define NUM_QUERY_BUCKETS 4
can you make this a static constexpr inside the Template class to limit the 
scope?


http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@35
PS6, Line 35: /// access to each shard of the map.
Update - it's not a single spinlock, but a spinlock per shard.

Add:
The underlying shard is locked and accessed by instantiating a 
ScopedShardedMapRef.


http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@43
PS6, Line 43: Template
I don't think we usually include the word Template since it's obvious by the <> 
in the use of the type.


http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@62
PS6, Line 62: map_
maps_ (plural) would help to clarify


http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@63
PS6, Line 63: map_lock_
map_locks_


http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@64
PS6, Line 64: };
you may end up with false sharing with this layout since locks might share 
cache lines. you'd probably be better off grouping each { map, lock} and using 
that as an element to the array (and making sure each shard is on its own 
cacheline).


http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@66
PS6, Line 66: /// Use this class to obtain a lock on a 
ShardedQueryMapTemplate[] for the duration of a
            : /// function/block, rather than doing so manually.
this needs updating. that thing is not an array, and it's not even possible to 
do it manually, and it would be good to explain what it's giving you a locked 
reference to. i.e. also say something about how this class gives you a 
reference to the underlying map that represents the shard.


http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@92
PS6, Line 92:   ~ScopedShardedMapRef() {
consider adding: map_lock_->DCheckLocked()


http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@96
PS6, Line 96: Returns the map corresponding to 'qs_map_bucket_'.
let's not explain public methods in terms of private fields. do something like:
Returns the shard (map) for the 'query_id' passed to the constructor.


http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@98
PS6, Line 98:   std::unordered_map<TUniqueId, T>* get() {
consider adding: map_lock_->DCheckLocked()


http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@117
PS6, Line 117:   uint8_t qs_map_bucket_;
why do we need that?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61089090e1095da45a8a64ed3ccc78bd310807f1
Gerrit-Change-Number: 8363
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Comment-Date: Fri, 09 Feb 2018 23:26:35 +0000
Gerrit-HasComments: Yes

Reply via email to