Обсуждение: [PATCH] Remove unncessary localtime() calls during data type conversion

Hi,

While doing some profiling work on an ODBC client, we noticed tons
of libc localtime() and __tz_convert() calls on a hot code path
(SQLFetch()), but most of the fields being retrieved were not of
date/time type.


These libc calls are costly (non-trivial work in terms of string ops,
timzone conversion etc.) and the cost adds up quickly for high-frequency
call like SQLFetch(). We noticed that this penalty is paid even
for data types which aren't date/time related. Code inspection
showed localtime() call in convert.c:copy_and_convert_field()
at function start, being invoked regardless of whether output of
localtime() is used or not.

Attached is a non-intrusive patch that moves the call down
to where it's needed (for both getting data from server and
sending data to server), but I'm not sure if the patch is complete
(it covers the case where source data type is time but
destination needs date part, "TIME->DATE[TIME]").

Could you please review this patch and modify it for correctness
and consider for committing it?

---

A longer term cleaner approach would be to refactor the code to aim for
at most one call to localtime() (e.g. if it's already invoked once
in copy_and_convert_field() before, reuse that data). However
that is much more intrusive change.

I checked the test/src/result-conversions-test.c code and the
test case specific to this scenario is commented out (since it
the expected result would depend on actual current date).
I un-commented those test cases, ran the test with and without
my patch and got same results. Also the data looks ok for
"TIME->DATE[TIME]" case, input is time, output is date.

   1114a1176
   > '13:23:34' (time) as SQL_C_TYPE_DATE: y: 2015 m: 6 d: 12

2015-6-12 is indeed current date.

Thanks,
  Nikhil

On Sat, Jun 13, 2015 at 9:42 AM, Nikhil Deshpande <ndeshpande@vmware.com> wrote:
> While doing some profiling work on an ODBC client, we noticed tons
> of libc localtime() and __tz_convert() calls on a hot code path
> (SQLFetch()), but most of the fields being retrieved were not of
> date/time type.
>
>
> These libc calls are costly (non-trivial work in terms of string ops,
> timzone conversion etc.) and the cost adds up quickly for high-frequency
> call like SQLFetch(). We noticed that this penalty is paid even
> for data types which aren't date/time related. Code inspection
> showed localtime() call in convert.c:copy_and_convert_field()
> at function start, being invoked regardless of whether output of
> localtime() is used or not.
>
> Attached is a non-intrusive patch that moves the call down
> to where it's needed (for both getting data from server and
> sending data to server), but I'm not sure if the patch is complete
> (it covers the case where source data type is time but
> destination needs date part, "TIME->DATE[TIME]").
>
> Could you please review this patch and modify it for correctness
> and consider for committing it?

Indeed. I would move as well the declarations of tim more within the
inner loops.

> A longer term cleaner approach would be to refactor the code to aim for
> at most one call to localtime() (e.g. if it's already invoked once
> in copy_and_convert_field() before, reuse that data). However
> that is much more intrusive change.

Yep. This would be saner long-term.
--
Michael

On 06/14/2015 06:36 AM, Michael Paquier wrote:
> On Sat, Jun 13, 2015 at 9:42 AM, Nikhil Deshpande <ndeshpande@vmware.com> wrote:
>> While doing some profiling work on an ODBC client, we noticed tons
>> of libc localtime() and __tz_convert() calls on a hot code path
>> (SQLFetch()), but most of the fields being retrieved were not of
>> date/time type.
>>
>>
>> These libc calls are costly (non-trivial work in terms of string ops,
>> timzone conversion etc.) and the cost adds up quickly for high-frequency
>> call like SQLFetch(). We noticed that this penalty is paid even
>> for data types which aren't date/time related. Code inspection
>> showed localtime() call in convert.c:copy_and_convert_field()
>> at function start, being invoked regardless of whether output of
>> localtime() is used or not.
>>
>> Attached is a non-intrusive patch that moves the call down
>> to where it's needed (for both getting data from server and
>> sending data to server), but I'm not sure if the patch is complete
>> (it covers the case where source data type is time but
>> destination needs date part, "TIME->DATE[TIME]").
>>
>> Could you please review this patch and modify it for correctness
>> and consider for committing it?
>
> Indeed. I would move as well the declarations of tim more within the
> inner loops.

This changes behaviour, when converting from text to date column.
Without this patch, if the string you're converting from is missing a
year/month/date field, it will be filled with the current date. This
modification to the result-conversions test case shows the difference:

--- a/test/src/result-conversions-test.c
+++ b/test/src/result-conversions-test.c
@@ -626,6 +626,8 @@ int main(int argc, char **argv)
      test_conversion("float8", "Infinity", SQL_C_DOUBLE, "SQL_C_DOUBLE", 20);
      test_conversion("float8", "-Infinity", SQL_C_DOUBLE, "SQL_C_DOUBLE", 20);

+    test_conversion("text", "", SQL_C_TYPE_DATE, "SQL_C_TYPE_DATE", 20);
+
      /* Clean up */
      test_disconnect();


We don't currently contain tests like that in the test suite, as
mentioned in the comment earlier in result-conversions-test.c:

>     /*
>      * Converting arbitrary things to date and timestamp produces results that
>      * depend on the current timestamp, because the driver substitutes the
>      * current year/month/datefor missing values. Disable for now, to get a
>      * reproducible result.
>      */

Would be nice to include those tests, perhaps by printing the difference
to current date instead of the absolute date.

- Heikki

On Tue, Sep 1, 2015 at 6:11 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> This changes behaviour, when converting from text to date column. Without
> this patch, if the string you're converting from is missing a
> year/month/date field, it will be filled with the current date.

Waking up an old thread, after being reminded by some colleagues that
there are no improvements in the upstream code yet regarding that...

> This modification to the result-conversions test case shows the difference:
>
> --- a/test/src/result-conversions-test.c
> +++ b/test/src/result-conversions-test.c
> @@ -626,6 +626,8 @@ int main(int argc, char **argv)
>         test_conversion("float8", "Infinity", SQL_C_DOUBLE, "SQL_C_DOUBLE",
> 20);
>         test_conversion("float8", "-Infinity", SQL_C_DOUBLE, "SQL_C_DOUBLE",
> 20);
>
> +       test_conversion("text", "", SQL_C_TYPE_DATE, "SQL_C_TYPE_DATE", 20);
> +
>         /* Clean up */
>         test_disconnect();
>
> We don't currently contain tests like that in the test suite, as mentioned
> in the comment earlier in result-conversions-test.c:

Yes, I can see the difference. I am noticing that only SQL_C_TYPE_DATE
and SQL_C_TYPE_TIMESTAMP are actually converting the non-defined
fields using the localtime values. SQL_C_TYPE_TIME is not impacted at
all. Is that expected?

>>         /*
>>          * Converting arbitrary things to date and timestamp produces
>> results that
>>          * depend on the current timestamp, because the driver substitutes
>> the
>>          * current year/month/datefor missing values. Disable for now, to
>> get a
>>          * reproducible result.
>>          */
>
>
> Would be nice to include those tests, perhaps by printing the difference to
> current date instead of the absolute date.

Yeah, I have done that in the attached with 0001, however I don't
really see how we can actually remove those comments, because some of
the tests include a fixed timestamp, and need to use a fixed
timestamp.

I am attaching patch 0002 that is a rebase of the main patch for HEAD,
that introduces no regressions based on the tests I have added in
0001. 0002 is proving to improve performance of the ODBC driver by
more or less 20% by reducing those localtime() calls in perf specs for
an application that is pretty hot with SQLFetch. That's quite
something.
--
Michael

Hi Michael,

Sorry for the late reply.

On 2016/04/25 15:30, Michael Paquier wrote:
> On Tue, Sep 1, 2015 at 6:11 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> This changes behaviour, when converting from text to date column. Without
>> this patch, if the string you're converting from is missing a
>> year/month/date field, it will be filled with the current date.
> Waking up an old thread, after being reminded by some colleagues that
> there are no improvements in the upstream code yet regarding that...
>
>> This modification to the result-conversions test case shows the difference:
>>
>> --- a/test/src/result-conversions-test.c
>> +++ b/test/src/result-conversions-test.c
>> @@ -626,6 +626,8 @@ int main(int argc, char **argv)
>>          test_conversion("float8", "Infinity", SQL_C_DOUBLE, "SQL_C_DOUBLE",
>> 20);
>>          test_conversion("float8", "-Infinity", SQL_C_DOUBLE, "SQL_C_DOUBLE",
>> 20);
>>
>> +       test_conversion("text", "", SQL_C_TYPE_DATE, "SQL_C_TYPE_DATE", 20);
>> +
>>          /* Clean up */
>>          test_disconnect();
>>
>> We don't currently contain tests like that in the test suite, as mentioned
>> in the comment earlier in result-conversions-test.c:
> Yes, I can see the difference. I am noticing that only SQL_C_TYPE_DATE
> and SQL_C_TYPE_TIMESTAMP are actually converting the non-defined
> fields using the localtime values. SQL_C_TYPE_TIME is not impacted at
> all. Is that expected?
>
>>>          /*
>>>           * Converting arbitrary things to date and timestamp produces
>>> results that
>>>           * depend on the current timestamp, because the driver substitutes
>>> the
>>>           * current year/month/datefor missing values. Disable for now, to
>>> get a
>>>           * reproducible result.
>>>           */
>>
>> Would be nice to include those tests, perhaps by printing the difference to
>> current date instead of the absolute date.
> Yeah, I have done that in the attached with 0001, however I don't
> really see how we can actually remove those comments, because some of
> the tests include a fixed timestamp, and need to use a fixed
> timestamp.
>
> I am attaching patch 0002 that is a rebase of the main patch for HEAD,
> that introduces no regressions based on the tests I have added in
> 0001. 0002 is proving to improve performance of the ODBC driver by
> more or less 20% by reducing those localtime() calls in perf specs for
> an application that is pretty hot with SQLFetch. That's quite
> something.

I would take care of  the patches.
Thanks.

regards,
Hiroshi Inoue

Hi Inoue-san,

On Fri, Apr 29, 2016 at 7:58 AM, Inoue, Hiroshi
<h-inoue@dream.email.ne.jp> wrote:
> On 2016/04/25 15:30, Michael Paquier wrote:
>> I am attaching patch 0002 that is a rebase of the main patch for HEAD,
>> that introduces no regressions based on the tests I have added in
>> 0001. 0002 is proving to improve performance of the ODBC driver by
>> more or less 20% by reducing those localtime() calls in perf specs for
>> an application that is pretty hot with SQLFetch. That's quite
>> something.
>
> I would take care of  the patches.

Thanks! If you need any help regarding those things, just don't
hesitate to ping me. I can easily allocate even company time for this
stuff.
--
Michael

On Fri, Apr 29, 2016 at 9:15 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Thanks! If you need any help regarding those things, just don't
> hesitate to ping me. I can easily allocate even company time for this
> stuff.

I am really late at the party here... My two patches has been
committed as 34cdbd1f and c2794dbb. Thanks Inoue-san for pushing them!
--
Michael