Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8345 )
Change subject: KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager ...................................................................... Patch Set 1: > Patch Set 1: > > > Patch Set 1: > > > > I dunno, I'm sort of leaning more towards the idea that true spin-locks > > should only be used for stuff as simple as "read this POD variable" and not > > for anything that involves acquiring other locks or allocating memory. > > The issue being that even if you address the acquisition sites I listed, we > still allocate memory while holding the TSTabletManager lock? I don't think > that's a huge deal, or, if it is, it's certainly a pattern we use widely > (i.e. take a spinlock, make a local copy of this map, release the spinlock). Yea, I think we should probably move away from such patterns as much as we can, especially if the allocation is unbounded. If we're talking about allocating a 4kb object it's most likely going to hit the local thread cache and be very fast, but I've found recently that allocations 1MB and larger can easily be tens or hundreds of milliseconds and in some cases even cross into >1s when things are under load. Moving to an overall better lock implementation should reduce our cognitive load here quite a bit - a good lock like absl::Mutex should have the same performance as a spinlock but also be smart enough to go to sleep when necessary instead of spinning. -- To view, visit http://gerrit.cloudera.org:8080/8345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I763abddd74d8b1dabb618318dc84256b533077e3 Gerrit-Change-Number: 8345 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Fri, 20 Oct 2017 21:56:37 +0000 Gerrit-HasComments: No
