maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxxx>]

Hi, Michael!

On May 19, Michael Widenius wrote:

> I don't remember any tests where you tested prepare stmt and
> microseconds. Did you add such a test somewhere else?

I've run everything with --ps-protocol

> >> > -void Field_timestamp::set_time()
> >> > +int Field_timestamp::set_time()
> >> >  {
> >> >    THD *thd= table ? table->in_use : current_thd;
> >> > -  long tmp= (long) thd->query_start();
> >> >    set_notnull();
> >> > -  store_timestamp(tmp);
> >> > +  store_TIME(thd->query_start(), 0);
> >> > +  return 0;
> >> >  }
> >> 
> >> The set_time() function could be a void as it can never fail.
> >> (Yes, I agree that it means that we have to test type of field in
> >> event code, but that something we should do at event startup
> >> anyway...)
> 
> > I may do it after a merge - I've seen that Alik added this test to
> > MySQL.
> 
> In other words, I have to do this when I do the merge...

I mean, 5.5 merge. So I will do it :)

> >> > +static void store_native(ulonglong num, uchar *to, uint bytes)
> >> >  {
> >> > +  switch(bytes) {
> >> > +  case 1: *to= (uchar)num;              break;
> >> > +  case 2: shortstore(to, (ushort)num);  break;
> >> > +  case 3: int3store(to, num); /* Sic!*/ break;
> >> > +  case 4: longstore(to, (ulong)num);    break;
> >> > +  case 8: longlongstore(to, num);       break;
> >> > +  default: DBUG_ASSERT(0);
> >> > +  }
> >> > +}
> >> 
> >> What would be even better if each field would have a pointers to c
> >> functions with the right read and write functions; Would avoid a
> >> switch for every call...
> 
> > Mats Kindal benchmarked and blogged about it.
> > switch is about 20% slower than function pointers if the function is
> > big. But it is several times faster than function pointers if the
> > function (with the switch) is small and can be inlined.
> 
> If 'bytes' would be a constant, I would agree with you
> Here it can't be efficently inlined as the caller don't use a constant
> for bytes.
> 
> Inline:in any function (especially one that has jumps) that generates
> > 60 bytes many times will make the code slower, now faster.
> (Takes more memory in the cpu, more memory for jump predictions etc).
> 
> Having one function that is called by many threads uses less CPU
> memory and is becasue of this usually faster then inline (when inline
> can't remove code because of usage of constants)

Here's what gcc is doing at -O3 (on my x86 laptop):

1. The function *is* inlined. And, sorry Monty, I prefer to trust gcc
   that inlining makes sense in this case.
2. if() is completely optimized away, just as good as #ifdef.
3. switch is implemented as a jump table:

        jmp     *.L2587(,%ecx,4)
         ...
.L2587:
        .long   .L2581
        .long   .L2582
        .long   .L2583
        .long   .L2584
        .long   .L2585
        .long   .L2581
        .long   .L2581
        .long   .L2581
        .long   .L2586

it's just an indirect jump, which is no worse than indirect call that
you'd get with a function pointer.

> >> > +  if (!tmp)
> >> > +    return fuzzydate & TIME_NO_ZERO_DATE;
> >> 
> >> should be:
> >> 
> >> > +    return (fuzzydate & TIME_NO_ZERO_DATE) != 0;
> >> 
> >> as the function is a bool function
> 
> > no need to because the function is a bool function. If it were my_bool,
> > then it would be needed.
> 
> Please add as otherwise we may get compiler warnings for loss of
> precession when converting to bool.

no :) I believe there can never be such a warning, it's not a "precision
loss" it's a check for truth value.

But ok, I promise to change it when I see a warning. In any compiler.

> >> > === modified file 'sql/log_event.cc'
> >> 
> >> >      if ((tmp_thd= current_thd))
> >> 
> >> current_thd() should never fail. If it can, there should be a comment
> >> in the code about this!
> 
> > This is your code :)
> > http://bazaar.launchpad.net/~maria-captains/maria/5.1/revision/2476.293.2
> > And there was a comment that dissapeared in that change.
> > It used to be
> 
> >      THD *thd= current_thd;
> >      /* thd will only be 0 here at time of log creation */
> >      now= thd ? thd->start_time : my_time(0);
> 
> And in my code, there was a comment, which was what I requested ;)
> I assume you added it back
 
No, the comment was before that change of yours, and you deleted it.
But I can add it back, no problem :)

> >> > === modified file 'sql/mysql_priv.h'
> 
> <cut>
> 
> >> > @@ -3045,10 +3045,10 @@ void sys_var_microseconds::set_default(T
> >> >  uchar *sys_var_microseconds::value_ptr(THD *thd, enum_var_type type,
> >> >                                            LEX_STRING *base)
> >> >  {
> >> > -  thd->tmp_double_value= (double) ((type == OPT_GLOBAL) ?
> >> > +  thd->sys_var_tmp.double_value= (double) ((type == OPT_GLOBAL) ?
> >> >                                     global_system_variables.*offset :
> >> >                                     thd->variables.*offset) / 1000000.0;
> >> 
> >> Change 100000.0 to same define constant as 1e6
> 
> > No. I have a constant for "precision of my_hrtime_t", it's 1000000,
> > which means "microseconds", but it does not have to be. We can have
> > my_hrtime_t storing, for example, hundreds of nanoseconds.
> 
> > I have a constant for "precision of second_part", it's also 1000000,
> > which means second_part is microseconds, but it doesn't have to be
> > either.
> 
> > But I won't introduce a constant for "number of microseconds in a
> > second".
> 
> And any reasons the above functions is returning microseconds ?

because we have a feature "microsecond precision in slow log", the
output is microseconds, variable is microseconds, everything is
documented to be microseconds. Various functions (like my_micro_time() -
now renamed to microsecond_interval_timer()) return microseconds,
variables (THD::start_utime, etc) store microseconds.

Technically I can change all that and introduce a constant (that affects
all that), but I thought it may be not worth the troubles.

Regards,
Sergei