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


Can you see any opportunity to break this diff apart?  As it stands i'm having 
a hard time giving a thoughtful review.  Perhaps you can start by introducing 
the `Assignment` class?


src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java
<https://reviews.apache.org/r/28617/#comment116667>

    What motivates you to supply these as overridden methods rather than a 
configuration object?  The latter seems like better encapsulation.


- Bill Farner


On Feb. 4, 2015, 11:38 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28617/
> -----------------------------------------------------------
> 
> (Updated Feb. 4, 2015, 11:38 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-909
>     https://issues.apache.org/jira/browse/AURORA-909
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Modified the task offer/task matching logic to skip offer matching for tasks 
> previously vetoed statically.
> 
> Real life testing in vagrant (see pictures) shows close to 50% improvement in 
> task scheduling performance.
> 
> Testing with JMH shows over 97% better perf when testing with disabled 
> preemptor (1 scheduling loop):
> ```
> Master
> Benchmark                                                                     
>          Mode  Samples        Score          Error    Units
> o.a.a.b.SchedulingBenchmarks.ConstraintMismatchsSchedulingBenchmark.runBenchmark
>        avgt      100    8291046.074 ±   145251.995  ns/op
> o.a.a.b.SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark
>      avgt      100    7522269.050 ±   142446.265  ns/op
> 
> This RB
> Benchmark                                                                     
>          Mode  Samples       Score           Error     Units
> o.a.a.b.SchedulingBenchmarks.ConstraintMismatchsSchedulingBenchmark.runBenchmark
>        avgt      100     204171.046 ±   3800.124  ns/op
> o.a.a.b.SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark
>      avgt      100     215854.129 ±   8959.851  ns/op
> ```
> 
> Testing with preemptor enabled and no tasks eligible for preemption gives 
> around 40% improvement (2 scheduling loops):
> ```
> Master
> Benchmark                                                                     
>          Mode  Samples          Score         Error  Units
> o.a.a.b.SchedulingBenchmarks.ConstraintMismatchsSchedulingBenchmark.runBenchmark
>        avgt      100    1767479.299 ±   26907.571  ns/op
> o.a.a.b.SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark
>      avgt      100    1538682.287 ±  119119.911  ns/op
> 
> This RB
> Benchmark                                                                     
>          Mode  Samples          Score         Error  Units
> o.a.a.b.SchedulingBenchmarks.ConstraintMismatchsSchedulingBenchmark.runBenchmark
>        avgt      100    1105731.141 ±   10040.721  ns/op
> o.a.a.b.SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark
>      avgt      100     939230.662 ±   11091.505  ns/op
> ```
> 
> Testing with preemptor enabled and running the worst case possible scenario 
> (every slave is eligible and all tasks are victims) yields the least 
> improvement 2-3% (3 scheduling loops).
> ```
> Master
> Benchmark                                                                     
>          Mode  Samples          Score        Error  Units
> o.a.a.b.SchedulingBenchmarks.ConstraintMismatchsSchedulingBenchmark.runBenchmark
>        avgt      100   11043701.243 ±  40550.259  ns/op
> o.a.a.b.SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark
>      avgt      100   10478631.055 ± 178833.158  ns/op
> o.a.a.b.SchedulingBenchmarks.PreemptorFallbackForLargeClusterBenchmark.runBenchmark
>     avgt      100  116258653.000 ± 403080.017  ns/op
> 
> This RB
> Benchmark                                                                     
>          Mode  Samples         Score         Error  Units
> o.a.a.b.SchedulingBenchmarks.ConstraintMismatchsSchedulingBenchmark.runBenchmark
>        avgt      100   10886116.889 ± 193934.324  ns/op
> o.a.a.b.SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark
>      avgt      100   10182572.955 ±  35740.891  ns/op
> o.a.a.b.SchedulingBenchmarks.PreemptorFallbackForLargeClusterBenchmark.runBenchmark
>     avgt      100  113656994.000 ± 424163.759  ns/op
> ```
> 
> 
> Diffs
> -----
> 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
> 8c11ef8bd6609f3e4d97ca154d922898f8362446 
>   src/jmh/java/org/apache/aurora/benchmark/Tasks.java 
> 1a35f9ee9e8e76def0f9bf5454cf8cbdf6a89c25 
>   src/main/java/org/apache/aurora/scheduler/TaskVars.java 
> f017cdd26ca40138a7e141f21613ed567314c399 
>   src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 
> f66383830140e5eaba436f35ebb5192eee65947a 
>   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
> ce47ff152e303fd2116bc3b9e91c0c1a8f76f258 
>   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
> 6a43bcd1719e8aa32fd3fcb7387d0318c3c0b804 
>   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
> f06fdaeb92e154d0982bdabed5df93e7bcba9048 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> e1c29747c9854cf75bf63f6f085cf40ca68989af 
>   src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
> 4e7efb3c1214c3d193afd61f162713490eb8effb 
>   src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 
> 4cf602ad32b972c18eb5a81e9b2f59c67859bdb2 
>   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 
> 5647349854a5e04de749c4d809684a0066d4da06 
>   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
> 6cc13231560996b144101eba36577f49017aba06 
>   
> src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
>  52ee7c1e3742d9315c7e7aaa77677121e1e9288d 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 411a55a8d85f60bb2703468f2d69b64b2736eee4 
> 
> Diff: https://reviews.apache.org/r/28617/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew -Pq build
> 
> Tested in vagrant
> 
> 
> File Attachments
> ----------------
> 
> NoStaticVetoFiltering.png
>   
> https://reviews.apache.org/media/uploaded/files/2014/12/03/7945c60b-4135-4016-a9bf-8d4815a4a573__NoStaticVetoFiltering.png
> StaticVetoFiltering.png
>   
> https://reviews.apache.org/media/uploaded/files/2014/12/03/2f73b94a-5ba9-43b6-922e-e9e4ec18d0bb__StaticVetoFiltering.png
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to