bzr commit into mysql-trunk-bugfixing branch (tor.didriksen:3254) WL#1393

[Tor Didriksen]

#At file:///export/home/didrik/repo/next-mr-opt-team-wl1393-merge/ based on revid:tor.didriksen@stripped

 3254 Tor Didriksen	2010-12-06
      WL#1393 Optimizing filesort with small limit
      
      More review comments.

    modified:
      sql/bounded_queue.h
      sql/filesort.cc
      unittest/gunit/bounded_queue-t.cc
=== modified file 'sql/bounded_queue.h'
--- a/sql/bounded_queue.h	2010-12-02 11:23:10 +0000
+++ b/sql/bounded_queue.h	2010-12-06 09:46:49 +0000
@@ -76,7 +76,6 @@ public:
     Initialize the queue.
 
     @param max_elements   The size of the queue.
-    @param offset_to_key  Offset to key in elements stored in the queue.
     @param max_at_top     Set to true if you want biggest element on top.
            false: We keep the n largest elements.
                   pop() will return the smallest key in the result set.
@@ -93,7 +92,7 @@ public:
 
     We do *not* take ownership of any of the input pointer arguments.
    */
-  int init(ha_rows max_elements, uint offset_to_key, bool max_at_top,
+  int init(ha_rows max_elements, bool max_at_top,
            compare_function compare, size_t compare_length,
            keymaker_function keymaker, Sort_param *sort_param,
            Key_type **sort_keys);
@@ -111,6 +110,10 @@ public:
     Removes the top element from the queue.
 
     @retval Pointer to the (key of the) removed element.
+
+    @note This function is for unit testing, where we push elements into to the
+          queue, and test that the appropriate keys are retained.
+          Interleaving of push() and pop() operations has not been tested.
    */
   Key_type **pop()
   {
@@ -144,7 +147,6 @@ private:
 
 template<typename Element_type, typename Key_type>
 int Bounded_queue<Element_type, Key_type>::init(ha_rows max_elements,
-                                                uint offset_to_key,
                                                 bool max_at_top,
                                                 compare_function compare,
                                                 size_t compare_length,
@@ -166,7 +168,7 @@ int Bounded_queue<Element_type, Key_type
       reinterpret_cast<compare_function>(get_ptr_compare(compare_length));
   // We allocate space for one extra element, for replace when queue is full.
   return init_queue(&m_queue, (uint) max_elements + 1,
-                    offset_to_key, max_at_top,
+                    0, max_at_top,
                     reinterpret_cast<queue_compare>(compare),
                     &m_compare_length);
 }

=== modified file 'sql/filesort.cc'
--- a/sql/filesort.cc	2010-12-02 11:23:10 +0000
+++ b/sql/filesort.cc	2010-12-06 09:46:49 +0000
@@ -44,11 +44,6 @@
 template class Bounded_queue<uchar, uchar>;
 #endif
 
-/// How to write record_ref.
-#define WRITE_REF(file,from) \
-if (my_b_write((file),(uchar*) (from),param->ref_length)) \
-  DBUG_RETURN(1);
-
 	/* functions defined in this file */
 
 static void make_char_array(FILESORT_INFO *info, uint fields, uint length);
@@ -228,8 +223,9 @@ ha_rows filesort(THD *thd, TABLE *table,
   {
     DBUG_PRINT("info", ("filesort PQ is applicable"));
     const size_t compare_length= param.sort_length;
-    if (pq.init(param.max_rows, 0, true,
-                NULL,
+    if (pq.init(param.max_rows,
+                true,                           // max_at_top
+                NULL,                           // compare_function
                 compare_length,
                 &make_sortkey, &param, table_sort.sort_keys))
     {
@@ -1188,6 +1184,7 @@ bool check_if_pq_applicable(Sort_param *
     }
   }
 
+  // Do we have space for LIMIT rows in memory?
   if (param->max_keys_per_buffer < num_available_keys)
   {
     make_char_array(filesort_info,
@@ -1210,7 +1207,6 @@ bool check_if_pq_applicable(Sort_param *
         get_merge_many_buffs_cost_fast(num_rows,
                                        num_available_keys,
                                        row_length);
-
       /*
         PQ has cost:
         (insert + qsort) * log(queue size) / TIME_FOR_COMPARE_ROWID +
@@ -1218,7 +1214,7 @@ bool check_if_pq_applicable(Sort_param *
       */
       const double pq_cost=
         (PQ_slowness * num_rows + param->max_keys_per_buffer) *
-        log(param->max_rows + 1.0) / TIME_FOR_COMPARE_ROWID +
+        log((double) param->max_keys_per_buffer) / TIME_FOR_COMPARE_ROWID +
         param->max_rows * table->file->scan_time();
 
       if (sort_merge_cost < pq_cost)

=== modified file 'unittest/gunit/bounded_queue-t.cc'
--- a/unittest/gunit/bounded_queue-t.cc	2010-12-02 11:23:10 +0000
+++ b/unittest/gunit/bounded_queue-t.cc	2010-12-06 09:46:49 +0000
@@ -170,7 +170,7 @@ TEST_F(BoundedQueueDeathTest, die_if_not
  */
 TEST_F(BoundedQueueDeathTest, die_if_popping_empty_queue)
 {
-  EXPECT_EQ(0, m_queue.init(0, 0, true, test_key_compare,
+  EXPECT_EQ(0, m_queue.init(0, true, test_key_compare,
                             m_key_size,
                             &test_keymaker, NULL, m_keys.key_ptrs));
   ::testing::FLAGS_gtest_death_test_style = "threadsafe";
@@ -185,7 +185,7 @@ TEST_F(BoundedQueueDeathTest, die_if_pop
  */
 TEST_F(BoundedQueueTest, construct_and_destruct)
 {
-  EXPECT_EQ(0, m_queue.init(num_elements/2, 0, true,
+  EXPECT_EQ(0, m_queue.init(num_elements/2, true,
                             test_key_compare,
                             m_key_size,
                             &test_keymaker, NULL, m_keys.key_ptrs));
@@ -197,11 +197,11 @@ TEST_F(BoundedQueueTest, construct_and_d
  */
 TEST_F(BoundedQueueTest, too_many_elements)
 {
-  EXPECT_EQ(1, m_queue.init(UINT_MAX, 0, true,
+  EXPECT_EQ(1, m_queue.init(UINT_MAX, true,
                             test_key_compare,
                             m_key_size,
                             &test_keymaker, NULL, m_keys.key_ptrs));
-  EXPECT_EQ(1, m_queue.init(UINT_MAX - 1, 0, true,
+  EXPECT_EQ(1, m_queue.init(UINT_MAX - 1, true,
                             test_key_compare,
                             m_key_size,
                             &test_keymaker, NULL, m_keys.key_ptrs));
@@ -213,7 +213,7 @@ TEST_F(BoundedQueueTest, too_many_elemen
  */
 TEST_F(BoundedQueueTest, zero_size_queue)
 {
-  EXPECT_EQ(0, m_queue.init(0, 0, true, test_key_compare,
+  EXPECT_EQ(0, m_queue.init(0, true, test_key_compare,
                             m_key_size,
                             &test_keymaker, NULL, m_keys.key_ptrs));
   insert_test_data();
@@ -226,7 +226,7 @@ TEST_F(BoundedQueueTest, zero_size_queue
  */
 TEST_F(BoundedQueueTest, push_and_pop_keep_largest)
 {
-  EXPECT_EQ(0, m_queue.init(num_elements/2, 0, false, test_key_compare,
+  EXPECT_EQ(0, m_queue.init(num_elements/2, false, test_key_compare,
                             m_key_size,
                             &test_keymaker, NULL, m_keys.key_ptrs));
   insert_test_data();
@@ -250,7 +250,7 @@ TEST_F(BoundedQueueTest, push_and_pop_ke
  */
 TEST_F(BoundedQueueTest, push_and_pop_keep_smallest)
 {
-  EXPECT_EQ(0, m_queue.init(num_elements/2, 0, true, test_key_compare,
+  EXPECT_EQ(0, m_queue.init(num_elements/2, true, test_key_compare,
                             m_key_size,
                             &test_keymaker, NULL, m_keys.key_ptrs));
   insert_test_data();
@@ -272,7 +272,7 @@ TEST_F(BoundedQueueTest, push_and_pop_ke
  */
 TEST_F(BoundedQueueTest, insert_and_sort)
 {
-  EXPECT_EQ(0, m_queue.init(num_elements/2, 0, true, test_key_compare,
+  EXPECT_EQ(0, m_queue.init(num_elements/2, true, test_key_compare,
                             m_key_size,
                             &test_keymaker, NULL, m_keys.key_ptrs));
   insert_test_data();
@@ -380,9 +380,9 @@ void insert_and_sort()
     Container *keys= new Container;
     srand(0);
     Bounded_queue<int, int> queue;
-    EXPECT_EQ(0, queue.init(limit, 0, true, int_ptr_compare,
+    EXPECT_EQ(0, queue.init(limit, true, int_ptr_compare,
                             sizeof(int), &int_keymaker, NULL, keys->key_ptrs));
-    for (int ix= 0; ix < limit; ++ix)
+    for (int ix= 0; ix < num_rows; ++ix)
     {
       int data= rand();
       queue.push(&data);


Attachment: [text/bzr-bundle] bzr/[e-mail]