----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71520/#review217860 -----------------------------------------------------------
Patch looks great! Reviews applied: [71519, 71520] Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh - Mesos Reviewbot On Sept. 19, 2019, 6:44 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71520/ > ----------------------------------------------------------- > > (Updated Sept. 19, 2019, 6:44 a.m.) > > > Review request for mesos, Benno Evers and Benjamin Mahler. > > > Bugs: MESOS-9948 > https://issues.apache.org/jira/browse/MESOS-9948 > > > Repository: mesos > > > Description > ------- > > This patch fixes some inefficient access patterns around `hashmap::get`. > Since this function returns an `Option` it can be used as a shorthand > for a `contains` check and subsequent creation of a value (`Option` > always contains a value). It was never not intended and is inefficient > for `contains` itself (e.g., via `hashmap::get::isSome`), and for cases > where only access to parts of the value in the `hashmap` is required > (e.g., to read a member of an optional value). In both these cases we > neither want nor need to create a temporary, and should instead either > just use `contains`, or access the value with `hashmap::at` after a > `contains` check since otherwise we might spend a lot of time creating > unnecessary temporary values. > > This patch fixes some easily identifiable case found by manually > grooming the result of the following clang-query command: > > match cxxMemberCallExpr( > on(hasType(cxxRecordDecl(hasName("hashmap")))), > unless( > hasParent(cxxBindTemporaryExpr( > hasParent(materializeTemporaryExpr( > hasParent(exprWithCleanups( > hasParent(varDecl())))))))), > callee(cxxMethodDecl(hasName("get")))) > > This most probably misses a lot of cases. Given how easy it is to > misuse `hashmap::get` we should consider whether it makes sense to get > rid of it completely in lie of an inlined form trading some additional > lookups for temporary avoidance, > > Option<X> x = map.contains(k) ? Some(map.at(k)) : Option<X>::none(); > > > Diffs > ----- > > 3rdparty/libprocess/src/metrics/metrics.cpp > 623d44adbe838f995ddbe89ee26f5bcc9c600be5 > > > Diff: https://reviews.apache.org/r/71520/diff/1/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
