Re: Review Request 71520: Fixed inefficient `hashmap` access patterns.
--- 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.
--- 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.
--- 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.
--- 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.
--- 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