> On Feb. 5, 2015, 12:01 a.m., Bill Farner wrote: > > 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? > > Maxim Khutornenko wrote: > I'd really prefer keeping this diff as a whole. The Assignment class > would not make sense without the entire picture in mind. > > Bill Farner wrote: > It's okay if you commit them back-to-back, but we really need to exercise > a pattern for incrementally building features even if they appear as code > islands in the commit history. Doing so makes it easier to review with > confidence, and more likely that you get faster review turnaround.
Splitting this RB into 4 parts for easier reviewing. Keeping it around for history though. - Maxim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28617/#review71079 ----------------------------------------------------------- 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 > >
