> On March 17, 2017, 1:33 a.m., Benjamin Mahler wrote: > > Hm.. this seems to introduce a performance regression in the case where > > there are many inactive roles in the system? I assume it's difficult to > > avoid in the hierarchical case? Otherwise, this looks good.
True -- if there are many inactive clients, we'll now calculate their share and include them in the `std::set`, whereas we wouldn't do so before (conversely, activating and deactivating clients is a lot faster than it used to be). My guess is that the bottlenecks in sorter performance likely lie elsewhere (e.g., updating resources, generating the entire `vector` in `sort()` when the allocator might only need the first few entries). We could avoid this overhead by skipping `calculateShare` for inactive clients, and then dirtying the whole sorter when a client is activated. My guess is that this isn't a net win (if you have a lot of inactive clients, it seems unfortunate to dirty the entire sorter whenever a client becomes active), but I can do some benchmarks if you think this case is important. > On March 17, 2017, 1:33 a.m., Benjamin Mahler wrote: > > src/master/allocator/sorter/drf/sorter.cpp > > Line 110 (original), 112-113 (patched) > > <https://reviews.apache.org/r/57564/diff/1/?file=1662778#file1662778line114> > > > > Why does it need to be re-inserted? Is this just to be prudent in case > > 'active' has an effect on the sorting? Or is it needed today? A comment > > would be great. The `std::set` interface doesn't let you modify elements directly. We could declare `active` to be `mutable`, which would avoid the need for the re-insertion. (This code is changed in the next review anyway.) > On March 17, 2017, 1:33 a.m., Benjamin Mahler wrote: > > src/master/allocator/sorter/drf/sorter.cpp > > Lines 156 (patched) > > <https://reviews.apache.org/r/57564/diff/1/?file=1662778#file1662778line163> > > > > Why the additional scope here? Seems a bit inconsistent Introducing a nested scope for distinct "steps" in a function can be helpful, IMO. In this case, we're introducing variables with names like `it` and `client`; it seems nice to avoid having those variables be in-scope for the entire function. - Neil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57564/#review169245 ----------------------------------------------------------- On March 13, 2017, 6:04 p.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57564/ > ----------------------------------------------------------- > > (Updated March 13, 2017, 6:04 p.m.) > > > Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park. > > > Repository: mesos > > > Description > ------- > > DRFSorter previously removed inactive clients from the `clients` > collection, and then re-added clients when they were reactivated. This > resulted in resetting the allocation count for the client, which is > unfortunate. This scheme would also be more difficult to adapt to > hierarchical sorting. > > This commit changes DRFSorter to continue to store inactive clients in > the `clients`; inactive clients are indicated by a new field in the > `Client` struct, and are omitted from the return value of > `DRFSorter::sort`. > > > Diffs > ----- > > src/master/allocator/sorter/drf/sorter.hpp > 76329220e1115c1de7810fb69b943c78c078be59 > src/master/allocator/sorter/drf/sorter.cpp > ed54680cecb637931fc344fbcf8fd3b14cc24295 > src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6 > > > Diff: https://reviews.apache.org/r/57564/diff/1/ > > > Testing > ------- > > `make check` > > > Thanks, > > Neil Conway > >
