Sailesh Mukil 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 7:

(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:
> can you make this a static constexpr inside the Template class to limit the
Done


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


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


http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@62
PS6, Line 62: RY_B
> maps_ (plural) would help to clarify
I moved this into a struct of arrays, so I kept the name as map_.


http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@63
PS6, Line 63:
> map_locks_
Same as above. I moved this into a struct of arrays, so I kept the name as 
map_lock_.


http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@64
PS6, Line 64:   // We group the map and its corresponding lock together to 
avoid false sharing. Since
> you may end up with false sharing with this layout since locks might share
Done


http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@66
PS6, Line 66:   // they can be allocated on the same cache line.
            :   struct MapShard {
> this needs updating. that thing is not an array, and it's not even possible
Done


http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@92
PS6, Line 92:       const TUniqueId& query_id, class ShardedQueryMap<T>* 
sharded_map) {
> consider adding: map_lock_->DCheckLocked()
Done


http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@96
PS6, Line 96: ap_lock_ = &sharded_map->shards_[qs_map_bucket].ma
> let's not explain public methods in terms of private fields. do something l
Done


http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@98
PS6, Line 98:     // Lock the corresponding shard.
> consider adding: map_lock_->DCheckLocked()
Done


http://gerrit.cloudera.org:8080/#/c/8363/6/be/src/util/sharded-query-map-util.h@117
PS6, Line 117:   }
> why do we need that?
Forgot to get rid of this. Done.



--
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: 7
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: Sat, 10 Feb 2018 00:57:44 +0000
Gerrit-HasComments: Yes

Reply via email to