-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71519/
-----------------------------------------------------------
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
-----
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/1/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
