UtcTimeStamp implementation bugs

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

UtcTimeStamp implementation bugs

Caleb Epstein-3
QuickFIX Documentation: http://www.quickfixengine.org/quickfix/doc/html/index.html
QuickFIX FAQ: http://www.quickfixengine.org/wikifix/index.php?QuickFixFAQ
QuickFIX Support: http://www.quickfixengine.org/services.html

Internally the FIX::UtcTimeStamp class is a "struct tm", a structure
that is generally intended to store times specified in your local
timezone.  By this I mean that if the structure has tm_hour = 10,
thats 10 AM in your local timezone, not in GMT.

Because FIX uses UTC time for everything, QuickFIX populates the
members of these structure with their UTC values, so tm_hour = 10
means 10 AM in UTC.  This is all well and good, but will break on
daylight savings time boundaries when coded as it is below (see my
comments):

  UtcTimeStamp( int hour, int minute, int second,
                int date, int month, int year )
  {
    // This is redundant and should be removed for efficiency
    setCurrent();  
    setHour( hour );
    setMinute( minute );
    setSecond( second );
    setMillisecond( 0 );
    setDate( date );
    setMonth( month );
    setYear( year );
    // mktime converts a struct tm (IN LOCAL TIME) to a time_t
    time_t t = mktime( (tm*)this );
    // Converts a time_t back to a struct tm.  Presumably this is done
    // for normalization so that a user specifying a date of 31-02-2005
    // will end up with a more correct 02-03-2005?
    *static_cast<tm*>(this) = time_localtime( &t );
    tm_isdst = -1;
  }

Where this code breaks is when you pass in a time that falls precisely
on a daylight savings time change-over.  For example, if you take the
*UTC* time 2005-04-03 2:00 AM and run it through this transformation,
here's what you get:

1. Intiialize struct tm: tm_year = 105, tm_mon = 3, tm_mday = 3,
tm_hour = 2, tm_min = 0, tm_sec = 0, tm_isdst = -1
2. Call mktime, yielding result = 1112511600 (given my local US/Eastern time)
3. Call localtime on this value, yielding: tm_year = 105, tm_mon = 3,
tm_mday = 3, tm_hour = *3*, tm_min = 0, tm_sec = 0, tm_isdst = 1

So we've actually turned 2 AM into 3 AM and we can't reverse this.

My impression is that the mktime and localtime calls (the latter of
which is redundant - POSIX mktime modifies the passed-in struct tm in
place so you don't need to call localtime at all) are used only to
normalize values that might be specified incorrectly by the user (e.g.
February 30th or the like)

But, because of their localtime semantics and the way that QuickFIX
populates the structures with GMT values, they actually cause breakage
in certain edge situations.  These functions are also performance pigs
on some platforms (notably Solaris before Solaris 9) where I've seen
QuickFIX spend ~50% of its runtime in mktime calls.

For these reasons, I'm going to submit a patch that drops the
mktime/localtime usage (and some unncessary setCurrent calls) from the
UtcTimeStamp class which should end up giving a small performance
boost.  Can anyone see a flaw in this analysis?

--
Caleb Epstein
caleb dot epstein at gmail dot com


-------------------------------------------------------
This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening
July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual
core and dual graphics technology at this free one hour event hosted by HP,
AMD, and NVIDIA.  To register visit http://www.hp.com/go/dualwebinar
_______________________________________________
Quickfix-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/quickfix-developers
Reply | Threaded
Open this post in threaded view
|

Re: UtcTimeStamp implementation bugs

Oren Miller
QuickFIX Documentation: http://www.quickfixengine.org/quickfix/doc/html/index.html
QuickFIX FAQ: http://www.quickfixengine.org/wikifix/index.php?QuickFixFAQ
QuickFIX Support: http://www.quickfixengine.org/services.html

I would be interested in seeing the patch.  I've never been comfortable with
the current implementation.

--oren

----- Original Message -----
From: "Caleb Epstein" <[hidden email]>
To: <[hidden email]>
Sent: Monday, July 11, 2005 5:38 PM
Subject: [Quickfix-developers] UtcTimeStamp implementation bugs


QuickFIX Documentation:
http://www.quickfixengine.org/quickfix/doc/html/index.html
QuickFIX FAQ: http://www.quickfixengine.org/wikifix/index.php?QuickFixFAQ
QuickFIX Support: http://www.quickfixengine.org/services.html

Internally the FIX::UtcTimeStamp class is a "struct tm", a structure
that is generally intended to store times specified in your local
timezone.  By this I mean that if the structure has tm_hour = 10,
thats 10 AM in your local timezone, not in GMT.

Because FIX uses UTC time for everything, QuickFIX populates the
members of these structure with their UTC values, so tm_hour = 10
means 10 AM in UTC.  This is all well and good, but will break on
daylight savings time boundaries when coded as it is below (see my
comments):

  UtcTimeStamp( int hour, int minute, int second,
                int date, int month, int year )
  {
    // This is redundant and should be removed for efficiency
    setCurrent();
    setHour( hour );
    setMinute( minute );
    setSecond( second );
    setMillisecond( 0 );
    setDate( date );
    setMonth( month );
    setYear( year );
    // mktime converts a struct tm (IN LOCAL TIME) to a time_t
    time_t t = mktime( (tm*)this );
    // Converts a time_t back to a struct tm.  Presumably this is done
    // for normalization so that a user specifying a date of 31-02-2005
    // will end up with a more correct 02-03-2005?
    *static_cast<tm*>(this) = time_localtime( &t );
    tm_isdst = -1;
  }

Where this code breaks is when you pass in a time that falls precisely
on a daylight savings time change-over.  For example, if you take the
*UTC* time 2005-04-03 2:00 AM and run it through this transformation,
here's what you get:

1. Intiialize struct tm: tm_year = 105, tm_mon = 3, tm_mday = 3,
tm_hour = 2, tm_min = 0, tm_sec = 0, tm_isdst = -1
2. Call mktime, yielding result = 1112511600 (given my local US/Eastern
time)
3. Call localtime on this value, yielding: tm_year = 105, tm_mon = 3,
tm_mday = 3, tm_hour = *3*, tm_min = 0, tm_sec = 0, tm_isdst = 1

So we've actually turned 2 AM into 3 AM and we can't reverse this.

My impression is that the mktime and localtime calls (the latter of
which is redundant - POSIX mktime modifies the passed-in struct tm in
place so you don't need to call localtime at all) are used only to
normalize values that might be specified incorrectly by the user (e.g.
February 30th or the like)

But, because of their localtime semantics and the way that QuickFIX
populates the structures with GMT values, they actually cause breakage
in certain edge situations.  These functions are also performance pigs
on some platforms (notably Solaris before Solaris 9) where I've seen
QuickFIX spend ~50% of its runtime in mktime calls.

For these reasons, I'm going to submit a patch that drops the
mktime/localtime usage (and some unncessary setCurrent calls) from the
UtcTimeStamp class which should end up giving a small performance
boost.  Can anyone see a flaw in this analysis?

--
Caleb Epstein
caleb dot epstein at gmail dot com


-------------------------------------------------------
This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening
July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual
core and dual graphics technology at this free one hour event hosted by HP,
AMD, and NVIDIA.  To register visit http://www.hp.com/go/dualwebinar
_______________________________________________
Quickfix-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/quickfix-developers



-------------------------------------------------------
This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening
July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual
core and dual graphics technology at this free one hour event hosted by HP,
AMD, and NVIDIA.  To register visit http://www.hp.com/go/dualwebinar
_______________________________________________
Quickfix-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/quickfix-developers
Reply | Threaded
Open this post in threaded view
|

Re: UtcTimeStamp implementation bugs

Caleb Epstein-3
In reply to this post by Caleb Epstein-3
QuickFIX Documentation: http://www.quickfixengine.org/quickfix/doc/html/index.html
QuickFIX FAQ: http://www.quickfixengine.org/wikifix/index.php?QuickFixFAQ
QuickFIX Support: http://www.quickfixengine.org/services.html

On 7/11/05, Caleb Epstein <[hidden email]> wrote:
>     // mktime converts a struct tm (IN LOCAL TIME) to a time_t
>     time_t t = mktime( (tm*)this );
>     // Converts a time_t back to a struct tm.  Presumably this is done
>     // for normalization so that a user specifying a date of 31-02-2005
>     // will end up with a more correct 02-03-2005?
>     *static_cast<tm*>(this) = time_localtime( &t );

[...]

> My impression is that the mktime and localtime calls (the latter of
> which is redundant - POSIX mktime modifies the passed-in struct tm in
> place so you don't need to call localtime at all) are used only to
> normalize values that might be specified incorrectly by the user (e.g.
> February 30th or the like)

Well, my impression was wrong :-)

There is code in SessionTime which uses the tm_wday (via getWeekDay)
so the call to mktime is necessary to populate this field, otherwise a
couple of tests in SessionTimeTest.cpp suite fail.

> But, because of their localtime semantics and the way that QuickFIX
> populates the structures with GMT values, they actually cause breakage
> in certain edge situations.  These functions are also performance pigs
> on some platforms (notably Solaris before Solaris 9) where I've seen
> QuickFIX spend ~50% of its runtime in mktime calls.
>
> For these reasons, I'm going to submit a patch that drops the
> mktime/localtime usage (and some unncessary setCurrent calls) from the
> UtcTimeStamp class which should end up giving a small performance
> boost.  Can anyone see a flaw in this analysis?

My approach now is to completely replace the underpinnings of
UtcTimeStamp and the related classes so they don't use "struct tm" at
all, but instead store a double representing the Julian day number and
the fractional portion of a day.  This eliminates the need for a lot
of the time-related system calls and static_cast<tm*> weirdness in
many places.

I'll send along the patch once I can get the code to stop failing the
bulk of the tests :-)

--
Caleb Epstein
caleb dot epstein at gmail dot com


-------------------------------------------------------
This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening
July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual
core and dual graphics technology at this free one hour event hosted by HP,
AMD, and NVIDIA.  To register visit http://www.hp.com/go/dualwebinar
_______________________________________________
Quickfix-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/quickfix-developers
Reply | Threaded
Open this post in threaded view
|

Re: UtcTimeStamp implementation bugs

Caleb Epstein-3
Ok, as threatened, here is the patch.

This patch replaces the underlying implementation of the UtcTimeStamp,
UtcDateOnly and UtcTimeOnly classes with a new class called DateTime.

The impetus for this is to fix potential daylight savings time bugs in
the previous implementation and reduce the dependence on the system
calls mktime() and localtime(), which are real performance pigs on
some platforms (e.g. all versions of SunOS and Solaris prior to
Solaris 9).

The new DateTime class uses two integers to represent a point in time.
One stores a Julian day number and the other stores a number of
milliseconds since midnight.  This implementation uses far less memory
than its predecessor (8 bytes instead of 44 for struct tm on Linux),
allows the use of a much wider range of dates (no 2038 problem) and is
faster to boot.

I have tested this patch on Linux only so far, but it passes all user-
and acceptance-tests.  I don't have MySQL available to test the
changes to the MySQLStore and MySQLLog classes.

Detailed changes:

Field.h: make getValue and type conversion operators return non-const
values.  Returning const temporaries makes no sense.

FieldConvertors.h: use compound getYMD/getHMS to extract values.  Use
public methods to initialize Utc{TimeStamp,DateOnly,TimeOnly}, not
static_cast.  Don't create default-constructed variables (which hit
the system clock).

FieldTypes.cpp: removed UtcTimeStamp::setTime, ::operator+=.  Added
DateTime::now here.  Because this code is compiled into the library,
HAVE_FTIME no longer needs to be defined to create timestamps with
millisecond resolution (this closes several bugs in the bug tracker).
We try to use gettimeofday if HAVE_FTIME is not defined (we should
probably check for gettimeofday first - ftime is deprecated).

FieldTypes.h: added DateTime class.  Modified Utc* classes to inherit
from it.  Dropped namespace-global constant UTC_DAY.  Use enum values
from DateTime instead.

MySQLLog.cpp, MySQLStore.cpp: use public interface to UtcTimeStamp,
not casting.  UNTESTED.

tests:

FieldConvertorsTestCase.cpp: use aggregate setYMD/setHMS methods.
getYearDay went away (this is the only place it was ever used)

MessagesTestCase.cpp: 1900-01-00 is not a valid date.  Use 1900-01-01.

SessionTestCase.cpp, UtcTimeStampTestCase.cpp: Use DateTime::SECONDS_PER_DAY

--
Caleb Epstein
caleb dot epstein at gmail dot com

quickfix-1.10-datetime.diff (57K) Download Attachment