List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:April 27 2010 8:52pm
Subject:Re: new review needed Re: patches for BUG#50595 (diff -u format)
View as plain text  
Hello,

Olav Sandstaa a écrit, Le 27.04.2010 13:28:
> Hi Guilhem,
> 
> This patch looks fine and I prefer this over the initial patch. OK to 
> commit.

Thanks.

> On my destop machine (compiling with Sun Studio, running on OpenSolaris) 
> the time improvement for running your test case is about 14% percent.

Same here.
One thing which is amazing (to me): if I take backout_nj_sj_state() 
(function introduced in this patch):

static void backout_nj_sj_state(const table_map remaining_tables,
                                 const JOIN_TAB *tab)
  {
   TABLE_LIST *last_emb= tab->table->pos_in_table_list->embedding;
   JOIN *join= tab->join;
   while (last_emb)

the "index" test runs in 1305 ms. If I swap the two first lines (those 
defining "last_emb" and "join"), it takes 1340 ms (2.7% more) 
Repeatable. I noticed this because I innocently swapped them, thinking 
that code would be more "localized" if last_emb tested in while() is set 
just before (it would still be in a CPU register when time comes to test 
it), and it degraded performance. My interpretation, which may be very 
wrong as I haven't checked it, is when last_emb is set first:
- a request is made to read tab->table->pos_in_table_list->embedding 
from memory
- while this request is pending (memory has not yet "replied"), another 
request is made to read tab->join from memory
- when we come to the while(), the first request has been served.
Whereas when the two lines are swapped, the second request 
(tab->table->pos_in_table_list->embedding) has not yet been served when 
we come to the while(), and as the while() depends on it, it stalls a bit.
Such analysis has zero value as I haven't checked it, but I'm really 
amazed at how little things can make a difference :-(
Thread
patches for BUG#50595 (diff -u format)Guilhem Bichot18 Apr
  • Re: patches for BUG#50595 (diff -u format)Olav Sandstaa20 Apr
    • Re: patches for BUG#50595 (diff -u format)Guilhem Bichot23 Apr
      • Re: patches for BUG#50595 (diff -u format)Olav Sandstaa23 Apr
        • Re: patches for BUG#50595 (diff -u format)Tor Didriksen23 Apr
      • Re: patches for BUG#50595 (diff -u format)Roy Lyseng26 Apr
  • Re: patches for BUG#50595 (diff -u format)Roy Lyseng21 Apr
    • Re: patches for BUG#50595 (diff -u format)Guilhem Bichot26 Apr
    • Re: patches for BUG#50595 (diff -u format)Guilhem Bichot26 Apr
    • new review needed Re: patches for BUG#50595 (diff -u format)Guilhem Bichot27 Apr
      • Re: new review needed Re: patches for BUG#50595 (diff -u format)Olav Sandstaa27 Apr
        • Re: new review needed Re: patches for BUG#50595 (diff -u format)Guilhem Bichot27 Apr