Re: Review Request 69032: Optimized resources filter operation.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [69032]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On Oct. 19, 2018, 4:22 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69032/
> ---
> 
> (Updated Oct. 19, 2018, 4:22 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
> 
> 
> Bugs: MESOS-9325
> https://issues.apache.org/jira/browse/MESOS-9325
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `Resources::filter()` operation uses `add()` which
> scans the resources vector, a O(n) operation. This is not
> necessary. `filter()` operation should only remove `Resource`
> entries. This patch optimizes the performance by directly
> `push_back` the resource to the vector without scanning.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f0f3df579550d874e477e2d6e7d0e87df079358a 
>   src/v1/resources.cpp 4d2b64f6e30ef96b47fc0b72b00158864e245650 
> 
> 
> Diff: https://reviews.apache.org/r/69032/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> # Benchmarking:
> 
> ## Before
> 
> [ RUN  ] Resources_Filter_BENCHMARK_Test.Filters
> Took 29.29123ms to perform 5 'r.nonRevocable()' operations on cpus:1; 
> gpus:1; mem:128; disk:256
> Took 7.129952ms to perform 5 'r.revocable()' operations on cpus{REV}:1
> Took 28.54168ms to perform 5 'r.unreserved()' operations on cpus:1; 
> gpus:1; mem:128; disk:256
> Took 36.282329ms to perform 5 'r.reserved(role)' operations on 
> cpus(reservations: [(STATIC,role)]):1; gpus(reservations: [(STATIC,role)]):1; 
> mem(reservations: [(STATIC,role)]):128; disk(reservations: 
> [(STATIC,role)]):256
> [   OK ] Resources_Filter_BENCHMARK_Test.Filters (101 ms)
> 
> ## After
> 
> [ RUN  ] Resources_Filter_BENCHMARK_Test.Filters
> Took 9.518106ms to perform 5 'r.nonRevocable()' operations on cpus:1; 
> gpus:1; mem:128; disk:256
> Took 4.33245ms to perform 5 'r.revocable()' operations on cpus{REV}:1
> Took 8.500219ms to perform 5 'r.unreserved()' operations on cpus:1; 
> gpus:1; mem:128; disk:256
> Took 15.63636ms to perform 5 'r.reserved(role)' operations on 
> cpus(reservations: [(STATIC,role)]):1; gpus(reservations: [(STATIC,role)]):1; 
> mem(reservations: [(STATIC,role)]):128; disk(reservations: 
> [(STATIC,role)]):256
> [   OK ] Resources_Filter_BENCHMARK_Test.Filters (38 ms)
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69032: Optimized resources filter operation.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69032 was successfully built and tested.

Reviews applied: `['69032']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2489/mesos-review-69032

- Mesos Reviewbot Windows


On Oct. 19, 2018, 4:22 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69032/
> ---
> 
> (Updated Oct. 19, 2018, 4:22 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
> 
> 
> Bugs: MESOS-9325
> https://issues.apache.org/jira/browse/MESOS-9325
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `Resources::filter()` operation uses `add()` which
> scans the resources vector, a O(n) operation. This is not
> necessary. `filter()` operation should only remove `Resource`
> entries. This patch optimizes the performance by directly
> `push_back` the resource to the vector without scanning.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f0f3df579550d874e477e2d6e7d0e87df079358a 
>   src/v1/resources.cpp 4d2b64f6e30ef96b47fc0b72b00158864e245650 
> 
> 
> Diff: https://reviews.apache.org/r/69032/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> # Benchmarking:
> 
> ## Before
> 
> [ RUN  ] Resources_Filter_BENCHMARK_Test.Filters
> Took 29.29123ms to perform 5 'r.nonRevocable()' operations on cpus:1; 
> gpus:1; mem:128; disk:256
> Took 7.129952ms to perform 5 'r.revocable()' operations on cpus{REV}:1
> Took 28.54168ms to perform 5 'r.unreserved()' operations on cpus:1; 
> gpus:1; mem:128; disk:256
> Took 36.282329ms to perform 5 'r.reserved(role)' operations on 
> cpus(reservations: [(STATIC,role)]):1; gpus(reservations: [(STATIC,role)]):1; 
> mem(reservations: [(STATIC,role)]):128; disk(reservations: 
> [(STATIC,role)]):256
> [   OK ] Resources_Filter_BENCHMARK_Test.Filters (101 ms)
> 
> ## After
> 
> [ RUN  ] Resources_Filter_BENCHMARK_Test.Filters
> Took 9.518106ms to perform 5 'r.nonRevocable()' operations on cpus:1; 
> gpus:1; mem:128; disk:256
> Took 4.33245ms to perform 5 'r.revocable()' operations on cpus{REV}:1
> Took 8.500219ms to perform 5 'r.unreserved()' operations on cpus:1; 
> gpus:1; mem:128; disk:256
> Took 15.63636ms to perform 5 'r.reserved(role)' operations on 
> cpus(reservations: [(STATIC,role)]):1; gpus(reservations: [(STATIC,role)]):1; 
> mem(reservations: [(STATIC,role)]):128; disk(reservations: 
> [(STATIC,role)]):256
> [   OK ] Resources_Filter_BENCHMARK_Test.Filters (38 ms)
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69032: Optimized resources filter operation.

[Meng Zhu]

> On Oct. 16, 2018, 5:24 p.m., Benjamin Mahler wrote:
> > Can you also post the filter benchmark results?
> > 
> > https://github.com/apache/mesos/blob/1.7.0/src/tests/resources_tests.cpp#L3865-L3920

Done.


> On Oct. 16, 2018, 5:24 p.m., Benjamin Mahler wrote:
> > src/common/resources.cpp
> > Line 1550 (original), 1550 (patched)
> > 
> >
> > Consider reserving the result vector?

Good point.


> On Oct. 16, 2018, 5:24 p.m., Benjamin Mahler wrote:
> > src/common/resources.cpp
> > Line 1555 (original), 1555 (patched)
> > 
> >
> > Probably warrants a comment about why we just add it to the vector 
> > instead of adding?

Done.


- Meng


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


On Oct. 18, 2018, 9:22 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69032/
> ---
> 
> (Updated Oct. 18, 2018, 9:22 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
> 
> 
> Bugs: MESOS-9325
> https://issues.apache.org/jira/browse/MESOS-9325
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `Resources::filter()` operation uses `add()` which
> scans the resources vector, a O(n) operation. This is not
> necessary. `filter()` operation should only remove `Resource`
> entries. This patch optimizes the performance by directly
> `push_back` the resource to the vector without scanning.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f0f3df579550d874e477e2d6e7d0e87df079358a 
>   src/v1/resources.cpp 4d2b64f6e30ef96b47fc0b72b00158864e245650 
> 
> 
> Diff: https://reviews.apache.org/r/69032/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> # Benchmarking:
> 
> ## Before
> 
> [ RUN  ] Resources_Filter_BENCHMARK_Test.Filters
> Took 29.29123ms to perform 5 'r.nonRevocable()' operations on cpus:1; 
> gpus:1; mem:128; disk:256
> Took 7.129952ms to perform 5 'r.revocable()' operations on cpus{REV}:1
> Took 28.54168ms to perform 5 'r.unreserved()' operations on cpus:1; 
> gpus:1; mem:128; disk:256
> Took 36.282329ms to perform 5 'r.reserved(role)' operations on 
> cpus(reservations: [(STATIC,role)]):1; gpus(reservations: [(STATIC,role)]):1; 
> mem(reservations: [(STATIC,role)]):128; disk(reservations: 
> [(STATIC,role)]):256
> [   OK ] Resources_Filter_BENCHMARK_Test.Filters (101 ms)
> 
> ## After
> 
> [ RUN  ] Resources_Filter_BENCHMARK_Test.Filters
> Took 9.518106ms to perform 5 'r.nonRevocable()' operations on cpus:1; 
> gpus:1; mem:128; disk:256
> Took 4.33245ms to perform 5 'r.revocable()' operations on cpus{REV}:1
> Took 8.500219ms to perform 5 'r.unreserved()' operations on cpus:1; 
> gpus:1; mem:128; disk:256
> Took 15.63636ms to perform 5 'r.reserved(role)' operations on 
> cpus(reservations: [(STATIC,role)]):1; gpus(reservations: [(STATIC,role)]):1; 
> mem(reservations: [(STATIC,role)]):128; disk(reservations: 
> [(STATIC,role)]):256
> [   OK ] Resources_Filter_BENCHMARK_Test.Filters (38 ms)
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69032: Optimized resources filter operation.

[Meng Zhu]

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

(Updated Oct. 18, 2018, 9:22 p.m.)


Review request for mesos, Benjamin Mahler and Gastón Kleiman.


Changes
---

Added benchmarking results and address Ben's comments.


Bugs: MESOS-9325
https://issues.apache.org/jira/browse/MESOS-9325


Repository: mesos


Description (updated)
---

Currently, `Resources::filter()` operation uses `add()` which
scans the resources vector, a O(n) operation. This is not
necessary. `filter()` operation should only remove `Resource`
entries. This patch optimizes the performance by directly
`push_back` the resource to the vector without scanning.


Diffs (updated)
-

  src/common/resources.cpp f0f3df579550d874e477e2d6e7d0e87df079358a 
  src/v1/resources.cpp 4d2b64f6e30ef96b47fc0b72b00158864e245650 


Diff: https://reviews.apache.org/r/69032/diff/2/

Changes: https://reviews.apache.org/r/69032/diff/1-2/


Testing (updated)
---

make check

# Benchmarking:

## Before

[ RUN  ] Resources_Filter_BENCHMARK_Test.Filters
Took 29.29123ms to perform 5 'r.nonRevocable()' operations on cpus:1; 
gpus:1; mem:128; disk:256
Took 7.129952ms to perform 5 'r.revocable()' operations on cpus{REV}:1
Took 28.54168ms to perform 5 'r.unreserved()' operations on cpus:1; gpus:1; 
mem:128; disk:256
Took 36.282329ms to perform 5 'r.reserved(role)' operations on 
cpus(reservations: [(STATIC,role)]):1; gpus(reservations: [(STATIC,role)]):1; 
mem(reservations: [(STATIC,role)]):128; disk(reservations: [(STATIC,role)]):256
[   OK ] Resources_Filter_BENCHMARK_Test.Filters (101 ms)

## After

[ RUN  ] Resources_Filter_BENCHMARK_Test.Filters
Took 9.518106ms to perform 5 'r.nonRevocable()' operations on cpus:1; 
gpus:1; mem:128; disk:256
Took 4.33245ms to perform 5 'r.revocable()' operations on cpus{REV}:1
Took 8.500219ms to perform 5 'r.unreserved()' operations on cpus:1; gpus:1; 
mem:128; disk:256
Took 15.63636ms to perform 5 'r.reserved(role)' operations on 
cpus(reservations: [(STATIC,role)]):1; gpus(reservations: [(STATIC,role)]):1; 
mem(reservations: [(STATIC,role)]):128; disk(reservations: [(STATIC,role)]):256
[   OK ] Resources_Filter_BENCHMARK_Test.Filters (38 ms)


Thanks,

Meng Zhu

Re: Review Request 69032: Optimized resources filter operation.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69032 was successfully built and tested.

Reviews applied: `['69032']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2478/mesos-review-69032

- Mesos Reviewbot Windows


On Oct. 16, 2018, 11:16 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69032/
> ---
> 
> (Updated Oct. 16, 2018, 11:16 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
> 
> 
> Bugs: MESOS-9325
> https://issues.apache.org/jira/browse/MESOS-9325
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `Resources::filter()` operation uses `add()` which
> scans the resources vector, a O(n) operation. This is not
> necessary. `filter()` operation should only remove `Resource`
> entires. This patch optimizes the performance by directly
> `push_back` the resource to the vector without scanning.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f0f3df579550d874e477e2d6e7d0e87df079358a 
>   src/v1/resources.cpp 4d2b64f6e30ef96b47fc0b72b00158864e245650 
> 
> 
> Diff: https://reviews.apache.org/r/69032/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69032: Optimized resources filter operation.

[Benjamin Mahler]

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


Ship it!




Can you also post the filter benchmark results?

https://github.com/apache/mesos/blob/1.7.0/src/tests/resources_tests.cpp#L3865-L3920


src/common/resources.cpp
Line 1550 (original), 1550 (patched)


Consider reserving the result vector?



src/common/resources.cpp
Line 1555 (original), 1555 (patched)


Probably warrants a comment about why we just add it to the vector instead 
of adding?


- Benjamin Mahler


On Oct. 16, 2018, 11:16 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69032/
> ---
> 
> (Updated Oct. 16, 2018, 11:16 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
> 
> 
> Bugs: MESOS-9325
> https://issues.apache.org/jira/browse/MESOS-9325
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `Resources::filter()` operation uses `add()` which
> scans the resources vector, a O(n) operation. This is not
> necessary. `filter()` operation should only remove `Resource`
> entires. This patch optimizes the performance by directly
> `push_back` the resource to the vector without scanning.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f0f3df579550d874e477e2d6e7d0e87df079358a 
>   src/v1/resources.cpp 4d2b64f6e30ef96b47fc0b72b00158864e245650 
> 
> 
> Diff: https://reviews.apache.org/r/69032/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>