Sailesh Mukil has posted comments on this change. ( )

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

Patch Set 7:

File be/src/util/sharded-query-map-util.h:
PS6, Line 29:
> can you make this a static constexpr inside the Template class to limit the
PS6, Line 35: /// Usage pattern:
> Update - it's not a single spinlock, but a spinlock per shard.
PS6, Line 43:
> I don't think we usually include the word Template since it's obvious by th
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_.
PS6, Line 63:
> map_locks_
Same as above. I moved this into a struct of arrays, so I kept the name as 
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
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
PS6, Line 92:       const TUniqueId& query_id, class ShardedQueryMap<T>* 
sharded_map) {
> consider adding: map_lock_->DCheckLocked()
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
PS6, Line 98:     // Lock the corresponding shard.
> consider adding: map_lock_->DCheckLocked()
PS6, Line 117:   }
> why do we need that?
Forgot to get rid of this. Done.

To view, visit
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Comment-Date: Sat, 10 Feb 2018 00:57:44 +0000
Gerrit-HasComments: Yes

Reply via email to