Re: Review Request 71520: Fixed inefficient `hashmap` access patterns.

2019-09-20 Thread Mesos Reviewbot

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71520/#review217890
---



Bad patch!

Reviews applied: [71519, 71520]

Failed command: ['bash', '-c', "set -o pipefail; 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 2>&1 | tee 
build_71520"]

Error:
..
ate_manager_process.hpp:929] Checkpointing UPDATE for operation status update 
OPERATION_FINISHED (Status UUID: 940f4d00-6695-45aa-a250-9507c3198a34) for 
operation UUID b3a97398-6410-4b63-b408-0eba2485da8b on agent 
534f9366-0c58-4e2d-9d37-74437bf8caa5-S0
I0920 14:06:46.514657 18812 status_update_manager_process.hpp:528] Forwarding 
operation status update OPERATION_FINISHED (Status UUID: 
940f4d00-6695-45aa-a250-9507c3198a34) for operation UUID 
b3a97398-6410-4b63-b408-0eba2485da8b on agent 
534f9366-0c58-4e2d-9d37-74437bf8caa5-S0
I0920 14:06:46.515743 18811 http_connection.hpp:131] Sending 
UPDATE_OPERATION_STATUS call to 
http://172.17.0.2:46739/slave(1246)/api/v1/resource_provider
I0920 14:06:46.516934 18803 process.cpp:3671] Handling HTTP event for process 
'slave(1246)' with path: '/slave(1246)/api/v1/resource_provider'
I0920 14:06:46.520047 18810 hierarchical.cpp:1786] Performed allocation for 1 
agents in 1.133837ms
I0920 14:06:46.520689 18809 master.cpp:10389] Sending offers [ 
534f9366-0c58-4e2d-9d37-74437bf8caa5-O3 ] to framework 
534f9366-0c58-4e2d-9d37-74437bf8caa5- (default) at 
scheduler-ff3c3bb8-0e3b-4e75-8daa-e248eac71088@172.17.0.2:46739
I0920 14:06:46.521351 18806 sched.cpp:934] Scheduler::resourceOffers took 
73637ns
I0920 14:06:46.524904 18802 process.cpp:3671] Handling HTTP event for process 
'master' with path: '/master/api/v1'
I0920 14:06:46.526823 18811 http.cpp:1115] HTTP POST for /master/api/v1 from 
172.17.0.2:44372
I0920 14:06:46.527076 18811 http.cpp:263] Processing call CREATE_VOLUMES
I0920 14:06:46.527822 18811 master.cpp:3938] Authorizing principal 
'test-principal' to create volumes 
'[{"disk":{"persistence":{"id":"ed7e02f4-402a-4a6b-b044-fd69d9bb8608","principal":"test-principal"},"source":{"id":"/tmp/CSIVersion_StorageLocalResourceProviderTest_OperatorOperationsWithResourceProviderResources_v1_N4Y1bn/2GB-2efda9be-e013-42ab-b96c-16f4db855b5b","mount":{"root":"./csi/org.apache.mesos.csi.test/local/mounts"},"profile":"test","type":"MOUNT","vendor":"org.apache.mesos.csi.test.local"},"volume":{"container_path":"volume","mode":"RW"}},"name":"disk","provider_id":{"value":"ba45d4df-aaea-477f-a68a-dd252d137e90"},"reservations":[{"role":"storage","type":"DYNAMIC"},{"principal":"test-principal","role":"storage/default-role","type":"DYNAMIC"}],"scalar":{"value":2048.0},"type":"SCALAR"}]'
I0920 14:06:46.529150 18810 sched.cpp:960] Rescinded offer 
534f9366-0c58-4e2d-9d37-74437bf8caa5-O3
I0920 14:06:46.529224 18810 sched.cpp:971] Scheduler::offerRescinded took 
23225ns
I0920 14:06:46.529758 18803 master.cpp:12684] Removing offer 
534f9366-0c58-4e2d-9d37-74437bf8caa5-O3
I0920 14:06:46.530614 18809 hierarchical.cpp:1511] Recovered ports(allocated: 
storage/default-role):[31000-32000]; disk(allocated: 
storage/default-role)(reservations: 
[(DYNAMIC,storage),(DYNAMIC,storage/default-role,test-principal)])[MOUNT(org.apache.mesos.csi.test.local,/tmp/CSIVersion_StorageLocalResourceProviderTest_OperatorOperationsWithResourceProviderResources_v1_N4Y1bn/2GB-2efda9be-e013-42ab-b96c-16f4db855b5b,test)]:2048;
 cpus(allocated: storage/default-role):2; mem(allocated: 
storage/default-role):1024; disk(allocated: storage/default-role):1024 (total: 
cpus:2; mem:1024; disk:1024; ports:[31000-32000]; disk(reservations: 
[(DYNAMIC,storage),(DYNAMIC,storage/default-role,test-principal)])[MOUNT(org.apache.mesos.csi.test.local,/tmp/CSIVersion_StorageLocalResourceProviderTest_OperatorOperationsWithResourceProviderResources_v1_N4Y1bn/2GB-2efda9be-e013-42ab-b96c-16f4db855b5b,test)]:2048,
 offered or allocated: {}) on agent 534f9366-0c58-4e2d-9d37-74437bf8caa5-S0 
from framework 534f9
 366-0c58-4e2d-9d37-74437bf8caa5-
I0920 14:06:46.530709 18809 hierarchical.cpp:1558] Framework 
534f9366-0c58-4e2d-9d37-74437bf8caa5- filtered agent 
534f9366-0c58-4e2d-9d37-74437bf8caa5-S0 for 5secs
I0920 14:06:46.532670 18806 master.cpp:12557] Sending operation '' (uuid: 
67fdaabf-1737-4ade-a631-7ac65973a64b) to agent 
534f9366-0c58-4e2d-9d37-74437bf8caa5-S0 at slave(1246)@172.17.0.2:46739 
(6775cde5d992)
I0920 14:06:46.533531 18800 slave.cpp:4352] Ignoring new checkpointed resources 
and operations identical to the current version
I0920 14:06:46.536733 18803 provider.cpp:498] Received APPLY_OPERATION event
I0920 14:06:46.536770 18803 provider.cpp:1351] Received CREATE operation '' 
(uuid: 67fdaabf-1737-4ade-a631-7ac65973a64b)
I0920 

Re: Review Request 71520: Fixed inefficient `hashmap` access patterns.

2019-09-20 Thread Mesos Reviewbot

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71520/#review217879
---



Bad review!

Reviews applied: [71520, 71519]

Error:
Circular dependency detected for review 71519.Please fix the 'depends_on' field.

- Mesos Reviewbot


On Sept. 19, 2019, 3:44 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71520/
> ---
> 
> (Updated Sept. 19, 2019, 3: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
> -
> 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> 623d44adbe838f995ddbe89ee26f5bcc9c600be5 
> 
> 
> Diff: https://reviews.apache.org/r/71520/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71520: Fixed inefficient `hashmap` access patterns.

2019-09-19 Thread Joseph Wu

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71520/#review217874
---


Ship it!




Nit: There's a typo in the third-last line of the commit description
s/completely in lie of/completely in lieu of/

The typo is also present in the next review ;)

- Joseph Wu


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 = map.contains(k) ? Some(map.at(k)) : Option::none();
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> 623d44adbe838f995ddbe89ee26f5bcc9c600be5 
> 
> 
> Diff: https://reviews.apache.org/r/71520/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71520: Fixed inefficient `hashmap` access patterns.

2019-09-19 Thread Mesos Reviewbot

---
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 = map.contains(k) ? Some(map.at(k)) : Option::none();
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> 623d44adbe838f995ddbe89ee26f5bcc9c600be5 
> 
> 
> Diff: https://reviews.apache.org/r/71520/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Review Request 71520: Fixed inefficient `hashmap` access patterns.

2019-09-19 Thread Benjamin Bannier

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71520/
---

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


Diffs
-

  3rdparty/libprocess/src/metrics/metrics.cpp 
623d44adbe838f995ddbe89ee26f5bcc9c600be5 


Diff: https://reviews.apache.org/r/71520/diff/1/


Testing
---

`make check`


Thanks,

Benjamin Bannier