mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 69098: Added a benchmark to compare quota and nonquota allocation performance.
Date Wed, 05 Dec 2018 23:39:04 GMT

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


Fix it, then Ship it!




Thanks for updating the test comment and commit description! Much clearer now and I think
I would understand the need for a separate benchmark it if I were coming in fresh.

Did you happen to post perf stacks anywhere?


src/tests/hierarchical_allocator_benchmarks.cpp
Lines 664-666 (patched)
<https://reviews.apache.org/r/69098/#comment295923>

    No need to state the name of the variable in its comment:
    
    // Determines the the number of agents in the cluster...
    
    But if this all that it is, no need for a comment at all, it's clear from the name what
it represents.
    
    I can't quite understand the second sentence, perhaps you can clarify it? Intuitively
it's not clear why the number of agents would influence how many are needed to satisfy a quota.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 667 (patched)
<https://reviews.apache.org/r/69098/#comment295921>

    agentsPerRole?



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 669 (patched)
<https://reviews.apache.org/r/69098/#comment295922>

    frameworksPerRole?



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 673-683 (patched)
<https://reviews.apache.org/r/69098/#comment295924>

    Why do we need to store it as a member? Are we doing this anywhere else? I seem to recall
we just access GetParam from the test itself?



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 690 (patched)
<https://reviews.apache.org/r/69098/#comment295926>

    This would be clearer?
    
    ```
    // 10 roles, 10*2 = 20 agents, 10*2 = 20 frameworks.
    ```
    
    etc



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 690-694 (patched)
<https://reviews.apache.org/r/69098/#comment295927>

    periods?



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 766 (patched)
<https://reviews.apache.org/r/69098/#comment295928>

    Can you pre-increment as a habit? I'm not sure if the compiler can optimize away the post-increment
copying in the case of iterators, Metric objects, etc. So as a habit pre-incrementing seems
like the better approach to adhere to



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 779-780 (patched)
<https://reviews.apache.org/r/69098/#comment295929>

    It's certainly not pristine from the perspective of the drf sorter's allocation count
and metrics for example. So we probably don't want to say pristine here.
    
    While it's a bit brittle of an assumption, it seems ok to me for now with a note about
this potentially starting from a dirty state if we change things.
    
    Did you consider using the param for this?



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 793 (patched)
<https://reviews.apache.org/r/69098/#comment295930>

    Do we need these two start outputs? It seems like they clutter the output to me.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 804-807 (patched)
<https://reviews.apache.org/r/69098/#comment295931>

    Maybe state why we don't bother recovering the resources here?


- Benjamin Mahler


On Oct. 22, 2018, 10:39 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69098/
> -----------------------------------------------------------
> 
> (Updated Oct. 22, 2018, 10:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
> 
> 
> Bugs: MESOS-6630
>     https://issues.apache.org/jira/browse/MESOS-6630
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This benchmark evaluates the performance difference between nonquota
> and quota settings. In both settings, the same allocations are made
> for fair comparison. In particular, since the agent will always be
> allocated as a whole in nonquota settings, we should also avoid
> agent chopping in quota setting as well. Thus in this benchmark,
> quotas are only set to be multiples of whole agent resources.
> This is also why we have this dedicated benchmark for comparison
> rather than extending the existing quota benchmarks (which involves
> agent chopping).
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_benchmarks.cpp 6c6330e8cdbc705be322a7e2445b295c35ee6952

> 
> 
> Diff: https://reviews.apache.org/r/69098/diff/2/
> 
> 
> Testing
> -------
> 
> Result from optimized build on a multicore 2.2GHz machine:
> 
> Benchmark setup: 20 agents, 10 roles, 20 frameworks
> Made 20 allocations in 3.508676ms for nonquota roles
> Made 20 allocations in 7.901269ms for quota roles
> 
> Benchmark setup: 200 agents, 100 roles, 200 frameworks
> Made 200 allocations in 63.407391ms for nonquota roles
> Made 200 allocations in 279.002319ms for quota roles
> 
> Benchmark setup: 2000 agents, 1000 roles, 2000 frameworks
> Made 2000 allocations in 4.003802373secs for nonquota roles
> Made 2000 allocations in 20.503188535secs for quota roles
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message