From reviews-return-25091-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Sat Feb 27 00:00:55 2016 Return-Path: X-Original-To: apmail-mesos-reviews-archive@minotaur.apache.org Delivered-To: apmail-mesos-reviews-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 6D3891892B for ; Sat, 27 Feb 2016 00:00:55 +0000 (UTC) Received: (qmail 61501 invoked by uid 500); 27 Feb 2016 00:00:55 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 61474 invoked by uid 500); 27 Feb 2016 00:00:55 -0000 Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: reviews@mesos.apache.org Delivered-To: mailing list reviews@mesos.apache.org Received: (qmail 61457 invoked by uid 99); 27 Feb 2016 00:00:55 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 27 Feb 2016 00:00:55 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 740FD2E6BCC; Sat, 27 Feb 2016 00:00:54 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============5039675653695713668==" MIME-Version: 1.0 Subject: Re: Review Request 43635: Changed scalar resources to use fixed-point internally. From: Joris Van Remoortere To: Joris Van Remoortere , Michael Park Cc: Neil Conway , mesos Date: Sat, 27 Feb 2016 00:00:54 -0000 Message-ID: <20160227000054.1669.6774@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: Joris Van Remoortere X-ReviewGroup: mesos X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/43635/ X-Sender: Joris Van Remoortere X-ReviewBoard-ShipIt: 1 References: <20160226235248.28813.59877@reviews.apache.org> In-Reply-To: <20160226235248.28813.59877@reviews.apache.org> Reply-To: Joris Van Remoortere X-ReviewRequest-Repository: mesos --===============5039675653695713668== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43635/#review121007 ----------------------------------------------------------- Fix it, then Ship it! src/common/values.cpp (line 62) Can we add a comment explaining why we use this quotient, remainder approach? - Joris Van Remoortere On Feb. 26, 2016, 11:52 p.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43635/ > ----------------------------------------------------------- > > (Updated Feb. 26, 2016, 11:52 p.m.) > > > Review request for mesos, Joris Van Remoortere and Michael Park. > > > Bugs: MESOS-4687 > https://issues.apache.org/jira/browse/MESOS-4687 > > > Repository: mesos > > > Description > ------- > > Scalar resource values are represented using floating point. As a result, users > could see unexpected results when accepting offers and making reservations for > fractional resources: values like "0.1" cannot be precisely represented using > standard floating point, and the resource values returned to frameworks might > contain an unpredictable amount of roundoff error. > > This commit adjusts the master to use fixed-point when doing internal > computations on scalar resource values. The fixed-point format only supports > three decimal digits of precision: that is, fractional resource values like > "0.001" will be supported, but "0.0001" will not be. > > > Diffs > ----- > > docs/attributes-resources.md 818da8ab0c672144b02f526b2b805cf0505d2c7e > docs/upgrades.md 21faea8a3c152b15023d6fa69cde9382dac80c18 > include/mesos/mesos.proto 33f6b0838360b61db70a247e5d6dfb16af15aa06 > include/mesos/v1/mesos.proto 1b0e709e76f3f6b44ab0434c649c064e8866c8a1 > src/common/resources.cpp 529a1cd99707f8ce7bcc22889793d1eea04c3338 > src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 > src/master/allocator/mesos/hierarchical.cpp 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d > src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff > src/v1/resources.cpp 49dc3429f3b4b18e24f80052ed8be830df53b59d > src/v1/values.cpp 58ea9875804bf0287855a1e9855855e5e54de4c4 > > Diff: https://reviews.apache.org/r/43635/diff/ > > > Testing > ------- > > make check > > Manually verified that some of the floating point oddities in https://issues.apache.org/jira/browse/MESOS-4071 do not occur when this patch is applied, although I wasn't able to reproduce the crash described in that ticket. > > REVIEW NOTES: > > * We don't currently emit a warning when discarding additional digits of precision from input scalar resource values. Should we? That would require identifying all the points where a resource value is seemed to be "user-provided", and also runs the risk of generating a ton of log messages when an old framework is used. > * Similarly, if the user gives us a resource value and we don't do anything to it, we won't discard any additional precision that appears in the value -- the precision only gets discarded when we apply an operator like `+` or `-`. Unclear if we should trim additional precision from all scalar resource values more aggressively. > > PERFORMANCE: > > This is the performance of the `DeclineOffers` benchmark WITHOUT this RR applied (optimized build on my laptop): > > ``` > [ RUN ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers > Using 2000 slaves and 200 frameworks > round 0 allocate took 2.192425secs to make 200 offers > round 1 allocate took 2.221243secs to make 200 offers > round 2 allocate took 2.236314secs to make 200 offers > round 3 allocate took 2.224045secs to make 200 offers > round 4 allocate took 2.232822secs to make 200 offers > round 5 allocate took 2.264807secs to make 200 offers > round 6 allocate took 2.224853secs to make 200 offers > round 7 allocate took 2.224829secs to make 200 offers > round 8 allocate took 2.24862secs to make 200 offers > round 9 allocate took 2.2556secs to make 200 offers > round 10 allocate took 2.256616secs to make 200 offers > ``` > > And after applying this RR: > > ``` > [ RUN ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers > Using 2000 slaves and 200 frameworks > round 0 allocate took 2.267919secs to make 200 offers > round 1 allocate took 2.202843secs to make 200 offers > round 2 allocate took 2.20426secs to make 200 offers > round 3 allocate took 2.263887secs to make 200 offers > round 4 allocate took 2.266237secs to make 200 offers > round 5 allocate took 2.276957secs to make 200 offers > round 6 allocate took 2.291821secs to make 200 offers > round 7 allocate took 2.261839secs to make 200 offers > round 8 allocate took 2.325696secs to make 200 offers > round 9 allocate took 2.310469secs to make 200 offers > round 10 allocate took 2.21802secs to make 200 offers > ``` > > Which suggests to me that any performance hit is pretty minimal. > > > Thanks, > > Neil Conway > > --===============5039675653695713668==--