From: Tor Didriksen Date: June 1 2011 12:06pm Subject: bzr commit into mysql-trunk branch (tor.didriksen:3135) WL#5860 List-Archive: http://lists.mysql.com/commits/138537 Message-Id: <20110601120643.E3758270@atum07.norway.sun.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1301454947912957503==" --===============1301454947912957503== MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline #At file:///export/home/didrik/repo/trunk-review-jorgen-cost/ based on revid:tor.didriksen@stripped 3135 Tor Didriksen 2011-06-01 Review comments for WL#5860: Make COST_VECT a properly .. modified: sql/handler.cc sql/handler.h unittest/gunit/cost_estimate-t.cc === modified file 'sql/handler.cc' --- a/sql/handler.cc 2011-05-31 13:13:42 +0000 +++ b/sql/handler.cc 2011-06-01 12:06:38 +0000 @@ -4564,7 +4564,10 @@ handler::multi_range_read_info_const(uin else cost->add_io(read_time(keyno, n_ranges, total_rows) * Cost_estimate::IO_BLOCK_READ_COST); - cost->add_cpu((double)total_rows * ROW_EVALUATE_COST + 0.01); + cost->add_cpu(total_rows * ROW_EVALUATE_COST + 0.01); + // If either operand is double, the other is converted to double, + // and ROW_EVALUATE_COST is a floating-point literal of type double. + // So there should be no need for a (C-style) cast. } return total_rows; } === modified file 'sql/handler.h' --- a/sql/handler.h 2011-05-31 13:13:42 +0000 +++ b/sql/handler.h 2011-06-01 12:06:38 +0000 @@ -1052,7 +1052,8 @@ uint16 &mrr_persistent_flag_storage(rang char* &mrr_get_ptr_by_idx(range_seq_t seq, uint idx); /** - Used to store optimizer cost estimates + Used to store optimizer cost estimates. + A note here about 'copy CTOR' and 'assignement operator' ? */ class Cost_estimate { @@ -1077,26 +1078,26 @@ public: {} /** - Returns sum of time-consuming costs, i.e., not counting memory - cost + Returns sum of time-consuming costs, i.e., not counting memory cost. */ - double total_cost() { return io_cost + cpu_cost + import_cost; } - double get_io_cost() { return io_cost; } - double get_cpu_cost() { return cpu_cost; } - double get_import_cost() { return import_cost; } - double get_mem_cost() { return mem_cost; } + double total_cost() const { return io_cost + cpu_cost + import_cost; } + double get_io_cost() const { return io_cost; } + double get_cpu_cost() const { return cpu_cost; } + double get_import_cost() const { return import_cost; } + double get_mem_cost() const { return mem_cost; } /** Reset all costs to zero */ - void zero() { io_cost= cpu_cost= import_cost= mem_cost= 0; } + void zzzero() { io_cost= cpu_cost= import_cost= mem_cost= 0; } + // This one was unused (except for the unit test). /** Whether or not all costs in the object are zero @return true if all costs are zero, false otherwise */ - bool is_zero() + bool is_zero() const { if (io_cost || cpu_cost || import_cost || mem_cost) return false; @@ -1124,7 +1125,8 @@ public: return *this; } - const Cost_estimate operator+ (const Cost_estimate &other) + Cost_estimate operator+ (const Cost_estimate &other) + // we return object by value here, so the const prefix didn't make much sense. { Cost_estimate result= *this; result+= other; === modified file 'unittest/gunit/cost_estimate-t.cc' --- a/unittest/gunit/cost_estimate-t.cc 2011-05-31 13:13:42 +0000 +++ b/unittest/gunit/cost_estimate-t.cc 2011-06-01 12:06:38 +0000 @@ -18,48 +18,34 @@ #include "my_config.h" #include -//#include "item.h" -// #include "sql_class.h" -// #include "rpl_handler.h" // delegates_init() #include "handler.h" namespace { -class CostEstimateTest : public ::testing::Test -{ -protected: - /* - This is the part of the server global things which have to be initialized - for this (very simple) unit test. Presumably the list will grow once - we start writing tests for more advanced classes. - TODO: Move to a common library. - */ - static void SetUpTestCase() {} - static void TearDownTestCase() {} - - CostEstimateTest() {} - - virtual void SetUp() {} - virtual void TearDown() {} -}; - +// Your test fixture is impty, so you don't need it. +// You want EXPECT_DOUBLE_EQ, see: +// http://code.google.com/p/googletest/wiki/AdvancedGuide#Floating-Point_Comparison -TEST_F(CostEstimateTest, Basics) +TEST(CostEstimateTest, Basics) { Cost_estimate cv1; EXPECT_EQ(0, cv1.total_cost()); EXPECT_TRUE(cv1.is_zero()); - cv1.add_io(4.5); + // It would be good to give names to all these magic constants. + const double initial_io_cost= 4.5; + + cv1.add_io(initial_io_cost); EXPECT_FALSE(cv1.is_zero()); - EXPECT_EQ(4.5, cv1.total_cost()); + EXPECT_EQ(initial_io_cost, cv1.total_cost()); - cv1.add_cpu(3.3); + const double initial_cpu_cost= 3.3; + cv1.add_cpu(initial_cpu_cost); - EXPECT_EQ(3.3, cv1.get_cpu_cost()); - EXPECT_EQ(4.5, cv1.get_io_cost()); - EXPECT_EQ(7.8, cv1.total_cost()); + EXPECT_DOUBLE_EQ(initial_cpu_cost, cv1.get_cpu_cost()); + EXPECT_DOUBLE_EQ(initial_io_cost, cv1.get_io_cost()); + EXPECT_DOUBLE_EQ(initial_io_cost + initial_cpu_cost, cv1.total_cost()); EXPECT_EQ(0, cv1.get_mem_cost()); EXPECT_EQ(0, cv1.get_import_cost()); @@ -74,11 +60,11 @@ TEST_F(CostEstimateTest, Basics) EXPECT_EQ(20.3, cv1.total_cost()); EXPECT_FALSE(cv1.is_zero()); - cv1.zero(); - EXPECT_TRUE(cv1.is_zero()); +// cv1.zero(); +// EXPECT_TRUE(cv1.is_zero()); } -TEST_F(CostEstimateTest, Operators) +TEST(CostEstimateTest, Operators) { Cost_estimate cv_io; --===============1301454947912957503== MIME-Version: 1.0 Content-Type: text/bzr-bundle; charset="us-ascii"; name="bzr/tor.didriksen@stripped" Content-Transfer-Encoding: 7bit Content-Disposition: inline # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: tor.didriksen@stripped\ # k1shqx22qautq33a # target_branch: file:///export/home/didrik/repo/trunk-review-jorgen-\ # cost/ # testament_sha1: 310f7b8e59f0675899815a1191aac091f889520d # timestamp: 2011-06-01 14:06:43 +0200 # base_revision_id: tor.didriksen@stripped\ # hfghtms7ejicf94m # # Begin bundle IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWUDR5TQAA9JfgHAQWf////+m /+C////6YAjPvPd7bvbaQ9UljSxu3tldBIUT2OEok0aBMk2mmptMp5qmp6eqMQAZBoaA0BiMJRAT RPJppNU/SjRkNABoANAAAANBoImIp6jNTaCbUDQANog0BoaGgYhoCREQCTKHiZFPNJpPKP1GoNqe kNoI0ANPQRjhppghkNNMjJhANNAGE0aZMACBoJJAQBTwJoQjMqHpppqnoxqR+qNPUNAMDTRqaAUS 5PrqrsxH2f9f2pXKkTOqb50dBhhJLfFSUTwu1RnC+iEYumFUoNF/OcOh3a/lZCtmSNGQrgnq+iiK aJOXEVpwpCgLxsaDxMTYvZ7Nuv55y/3tsXxyXOv1WlMbE22lhy9X8Iy6sbpepnHMGt7Ha53TN6Lo 4nxGHaDYgrGGEBwYEc9IRoZhpwl4Jtm1PMzMZIYoGu7juaQuYLddNuzRRkc+PYsBGzgmnz9UmFV8 xUC9vYwHC1T4/Z7EXpOzb+KwC5U0nWtcMHRHnJu2wf+mYtWN+juqi4wZo9T/uSpK95JGS1qhBrAJ KVQSwhBwJjWJHXdVKCms9iy2zNLJsgQ8Qj/Bt3ad/Fi7hsOtG/3LBEaqCfKqu37sKDQGptjY55x/ m9v8lWTPzGGf0BmYGJxNmpk32sJSFR764dbfqacDYpY+gA7T6yHdTtlzDRPlNeEBHtM0vLvcR00O JrYGLYMZv7TzZU6R1yKRiAwROK+Ea1Gak1vSPMefgbghx2NYV3c26DDZmWfvcmKS8jI1OBNnli4a IQM5zCIQEHkhOkwJPEbqU84DE9YtHYDVl14pWNho8LGYV28hUkPXcHAwsbHJaf8B12yHGw/S0ohp oTSrXpMuSwLLhBAZQhPMkSnIQ8Y+uTdzX8LJImq62wQ68Ou3eWlkuGPZoEy4sW/PJUY7HQSWuzwz 1Ir6BDYo2aL+JxUNWbO1hBkIxOBefaIoH3QJFEamffc5/Fz9nCcamBUbH7DCLCVz3V05hmhyOJvs rEMaxWJRQUQEFfLemgYixKkRpUV5qcRkXlEtg2aubRRjeiKMmsTdYwU4kwr0cTJGoTx3jjq0G3Fa fTUYHAjriiGRxDM7lZerDniUZQr7FIhvWnM0JT1VbYoUosuadLUpiSEQnutJlRCLixibjHGSYGB5 mcDMuK7Ln3utGqiEqRwx2ZcTQxCo3ltaxxRWdZI3V3766IwQym0i6Cb+u0gVyTUaXTNl5KM8ltUs 6ZwcNcDZRcyIPM8pLJbTYQWUm5orgoWyEjFz8IQcYwMKx2I8g/lN2bKLV1OptaTKTBByGk0AmKy1 2hTeaWLU+i9hvDhhLLO22aBjDlT2b1u331VglHcdxbeZJjY5CvX1/EmWxRJJAOll3cKqPMMsFB+A d88eep7+cPUTd2L+w7SnwFs0o99C7wdpw8V/8wVOWTG7VEdqQloSDF2BJjhojBTSgQm/AQfZ2N8s dVrIoNsdIihMSOdgyoNGdpm6KUgYUFH2+ikik6TgTDta6vKfUXgPGpeQcku9xoBHwni+S5eU2dy0 hNf6rKpxoCrj5twOY9wu5NBfGV2Yh/h/vS5I6itfcIl55zd59J/Egbyh+8idI1LKDGh/U1v7RENO +I/xSDp6DBgb+gxcPOT25mJiOkVPgdosiJkYen43ie92Bk5kB9K86xBdo3my9ze32zP3vxeVQ0lS ISPVi7KHCVIaG3ymjwyGZrSaWrpGDFHK6wtMOX48BiOlQiQV0g1EgRl6a+kePMLWHHYOVWDsPqPm tewS9YYCpgkajgCrz9ndX41cUq0VCiKR9LnNhSj10rShh0ukWYwQERk2ceUxL52BsCi6br8fQkid 6BdCjGFKTiV7TGt1KxwUDWZ67B9Kxrca4X9q4XcGs97ixUNdmDJJ1YehOcjDbVbhdT8zsCKqdAKn VS5Te8+Azu5asPLxW3i/kMlUeqSVP7czcGvSW5GBAXEJfsR5JcpwhvBrS0605E2JOpkPYhyPKh3h gpBql0hR9LDNgoO3mMB8fHx7zM3WtJOPqAleNnMu81i25XTZ4BiOhWbElaRvg2tBrs8a0KdmG67u S5T/swlCvvHhR7K0lxQbzxKAVmSPUsxaOiu8iMDYjuXCdMmEzNeKtwc268hYbiHhxegw46HoLVCe 9XUp2CkTqgId6jmVHPIVg5JfA2lDyLgtE9w9iCFFSyPI7CpYpPZXgGbQF6RP1rgNwp+W3BYyuoJ9 AgwVZEiAzd5NNpFS1jwmYquL7iw7i++bwsDMO85llxmBdyW43inXWjadU1Ak7gyRQTQTI9dEYZEI HVUMFdaCutkbVrUOWh71DiX3LqZ/xaoJsYjhGpdyK9J0KJAyl976FQCP70aViq9kkLRdd0Uw0PXm sgD5IBkvWD07FGblMAOgGRq8Gq+WfL55iPD1VQyyee2/YV93ayZEEW3L72O88S8wOdAa9riws5Vw wSHOLxG49oj05GzMaBF6xXRnkltMSYmkh7GHAOlILbEGgWGHreEwn3ZpKjrIRWRYLIaAbzSq8aj7 EwSGiwZ6p7FcbcumUG2TTNoekano/7pxSTbBvbJPyEYWWiiqi5G2pSTU/cehma0wszWO5Mmg4TkD mGGKkQIHMQv2Gu+LKl0WUy0NXunDdIbaesc6PPCSIcsjLMSCwZXOFCUSoDrFYYhUthlGRdpJ1STB gnZVJPVh7R7XIEpAQsVpccKPBg9NTUGTvhSlGhT9x5Y7CI0tQ1zIvuzzSwnbzMiBYMgcI6e3+BtW fORTWTK/bhJ8XZAYzH6iKDrA5HYVYa1BoBkR6h45dLY3Ud4iX2HvclDoPA38Nh+5x6cyahtKykgw sgQy0q+sz3lhFMgncLuSKcKEggaPKaA= --===============1301454947912957503==--