----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71519/#review217886 -----------------------------------------------------------
Ship it! The review looks good, but this sentence from the commit message seems to be a bit mangled: > It was never not intended and is inefficient as 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 access a member of an optional value). - Benno Evers On Sept. 19, 2019, 1:44 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71519/ > ----------------------------------------------------------- > > (Updated Sept. 19, 2019, 1:44 p.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 > as `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 access 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; otherwise we might spend a lot of time creating > unnecessary temporary values. > > This patch fixes some easily identifiable cases found from skimming 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 lieu 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 > ----- > > src/exec/exec.cpp 67e082e709ef803f49646da5c36147158330f725 > src/executor/executor.cpp 664a2f1e0723f1afd9220e86e47e86cda67f6b9a > src/files/files.cpp f200d5e797084a2a5137ac8f8ede1f6243f12cfb > src/master/master.cpp a2c289a8b6e30eac690f77c7fc1b12b16f62f541 > src/master/metrics.cpp 20d7231888e263cb3c1759407a2476291e515d4a > src/slave/containerizer/fetcher.cpp > 8ae789a9f260645e574bbe46a108c3b8cab44cc4 > src/slave/containerizer/mesos/isolators/posix.hpp > 1ff942cfd1f6829bdc3661b9260dd8e53732d023 > src/slave/containerizer/mesos/launcher.cpp > 413cc621ad49150a6ddaf689bb75b9dc44741563 > src/slave/containerizer/mesos/provisioner/provisioner.cpp > bf3908d274f8b6f4a57998cbb0a62312b71e3856 > src/slave/slave.cpp 96890d37ec024fc33d2403b32576776888bcd9f7 > src/state/log.cpp d3bf2ccceff1934785889ddc9507a754cce445fe > src/tests/slave_recovery_tests.cpp d99752ab082f1aca9fb77659378d0bca5a0598eb > src/tests/task_status_update_manager_tests.cpp > 0c8b0586a760683ff0ab0f11bf658073087eac12 > > > Diff: https://reviews.apache.org/r/71519/diff/2/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
