---
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 = map.contains(k) ? Some(map.at(k)) : Option::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
>
>
