Derby
  1. Derby
  2. DERBY-4752

CheapDateFormatter returns incorrect and invalid date strings

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.7.1.1
    • Fix Version/s: 10.7.1.1
    • Component/s: Services
    • Labels:
      None

      Description

      CheapDateFormatter has multiple problems. These are the ones I'm aware of:

      1) On the boundary between non-leap years and leap years it will return first day of thirteenth month in previous year (for instance, 2011-13-01 instead of 2012-01-01)

      2) It treats all years divisible by four as leap years. Those divisible by 100 and not by 400 are not leap years. It attempts to adjust for that (see the snippet below) but it always ends up setting leapYear=true if (year%4)==0.

      // It's a leap year if divisible by 4, unless divisible by 100,
      // unless divisible by 400.
      if ((year % 4L) == 0) {
      if ((year % 100L) == 0) {
      if ((year % 400L) == 0)

      { leapYear = true; }

      }
      leapYear = true;
      }

      3) More leap year trouble. To find out which year it is, it calculates the number of four year periods that have elapsed since 1970-01-01. A four year period is considered 365*3+366 days. Although most four year periods are of that length, some are shorter, so we'll get one day off starting from year 2100, two days off from year 2200, and so on.

      1. remove-cheap-formatter.stat
        0.8 kB
        Knut Anders Hatlen
      2. remove-cheap-formatter.diff
        20 kB
        Knut Anders Hatlen
      3. derby-4752-1c.diff
        11 kB
        Knut Anders Hatlen
      4. derby-4752-1b.diff
        11 kB
        Knut Anders Hatlen
      5. derby-4752-1a.diff
        11 kB
        Knut Anders Hatlen
      6. CheapFormatterPerfTest.java
        0.9 kB
        Knut Anders Hatlen

        Issue Links

          Activity

          Hide
          Knut Anders Hatlen added a comment -

          Examples:

          Issue 1:

          CheapDateFormatter.formatDate(1325376000000L) returns "2011-13-01 00:00:00.000 GMT", expected: "2012-01-01 00:00:00.000 GMT"

          The returned date is wrong and invalid because there is no month 13.

          Issue 2:

          CheapDateFormatter.formatDate(4114195200000L) returns "2100-05-16 00:00:00.000 GMT", expected: "2100-05-17 00:00:00.000 GMT"

          The returned date is one day off.

          Issue 3:

          CheapDateFormatter.formatDate(4107542400000L) returns "2100-02-29 00:00:00.000 GMT", expected "2100-03-01 00:00:00.000 GMT"

          The returned date is invalid because there is no February 29 in the year 2100.

          Show
          Knut Anders Hatlen added a comment - Examples: Issue 1: CheapDateFormatter.formatDate(1325376000000L) returns "2011-13-01 00:00:00.000 GMT", expected: "2012-01-01 00:00:00.000 GMT" The returned date is wrong and invalid because there is no month 13. Issue 2: CheapDateFormatter.formatDate(4114195200000L) returns "2100-05-16 00:00:00.000 GMT", expected: "2100-05-17 00:00:00.000 GMT" The returned date is one day off. Issue 3: CheapDateFormatter.formatDate(4107542400000L) returns "2100-02-29 00:00:00.000 GMT", expected "2100-03-01 00:00:00.000 GMT" The returned date is invalid because there is no February 29 in the year 2100.
          Hide
          Knut Anders Hatlen added a comment -

          Instead of trying to fix the algorithm, I think it would be better to rely on java.util.Calendar to do these calculations for us. That would require allocating a Calendar object each time the method is called (in addition to all the String objects the method already allocates), but it won't instantiate any ResourceBundles or Locales, which was the reason why CheapDateFormatter was written in the first place, according to the comments. I haven't found that CheapDateFormatter is used in any performance critical section of the code, so I'm not sure if that's much of a concern in any case.

          Show
          Knut Anders Hatlen added a comment - Instead of trying to fix the algorithm, I think it would be better to rely on java.util.Calendar to do these calculations for us. That would require allocating a Calendar object each time the method is called (in addition to all the String objects the method already allocates), but it won't instantiate any ResourceBundles or Locales, which was the reason why CheapDateFormatter was written in the first place, according to the comments. I haven't found that CheapDateFormatter is used in any performance critical section of the code, so I'm not sure if that's much of a concern in any case.
          Hide
          Knut Anders Hatlen added a comment -

          Here's a patch that makes CheapDateFormatter.formatDate() use a Calendar object to calculate the components of the date. The patch also adds a unit test for the method. Running the full regression test suite now.

          Show
          Knut Anders Hatlen added a comment - Here's a patch that makes CheapDateFormatter.formatDate() use a Calendar object to calculate the components of the date. The patch also adds a unit test for the method. Running the full regression test suite now.
          Hide
          Knut Anders Hatlen added a comment -

          Whoops! I meant to fix a typo in a comment before I uploaded the patch. 1b fixes the typo and removes an unintended blank line.

          Show
          Knut Anders Hatlen added a comment - Whoops! I meant to fix a typo in a comment before I uploaded the patch. 1b fixes the typo and removes an unintended blank line.
          Hide
          Knut Anders Hatlen added a comment -

          Sigh... More typos... I had the wrong JIRA number in the test comments.

          Show
          Knut Anders Hatlen added a comment - Sigh... More typos... I had the wrong JIRA number in the test comments.
          Hide
          Knut Anders Hatlen added a comment -

          All the regression tests passed with the 1c patch.

          Show
          Knut Anders Hatlen added a comment - All the regression tests passed with the 1c patch.
          Hide
          Kathey Marsden added a comment -

          I am not really sure if this is at all relevant at this time, but this issue brought back memories because it touched on my first ever technical conversation regarding what was at that time Cloudscape with Jeff Lichtman. I seem to recall he had implemented CheapDateFormatter because it was found just printing the date to the log was taking huge amounts of time. Of course Calendar did not exist at that time, so I don't know if the performance is acceptable. Also I think any performance impact was probably more closely related to being able to test performance with statement logging, etc turned on than a typical user scenario, but I thought I would mention this bit of history in case performance of the new CheapDateFormatter needs to be measured.

          Show
          Kathey Marsden added a comment - I am not really sure if this is at all relevant at this time, but this issue brought back memories because it touched on my first ever technical conversation regarding what was at that time Cloudscape with Jeff Lichtman. I seem to recall he had implemented CheapDateFormatter because it was found just printing the date to the log was taking huge amounts of time. Of course Calendar did not exist at that time, so I don't know if the performance is acceptable. Also I think any performance impact was probably more closely related to being able to test performance with statement logging, etc turned on than a typical user scenario, but I thought I would mention this bit of history in case performance of the new CheapDateFormatter needs to be measured.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for providing the historical background for the CheapDateFormatter class, Kathey.

          I wrote a small performance test (see the attached CheapFormatterPerfTest.java) to see the impact of the fix. On my laptop, running Java 6u21 with default options, the old implementation managed to format about 280000 timestamps per second. The new implementation only manages about 190000 timestamps per second, which is slower, but I cannot imagine that it would be noticed in any realistic scenario.

          Show
          Knut Anders Hatlen added a comment - Thanks for providing the historical background for the CheapDateFormatter class, Kathey. I wrote a small performance test (see the attached CheapFormatterPerfTest.java) to see the impact of the fix. On my laptop, running Java 6u21 with default options, the old implementation managed to format about 280000 timestamps per second. The new implementation only manages about 190000 timestamps per second, which is slower, but I cannot imagine that it would be noticed in any realistic scenario.
          Hide
          Lily Wei added a comment -

          I am not totally understand the performance impact for this issue. However, (280000-190000)/280000 is 32% slower. If we apply that to a big amount of timestamp implementation store procedure. Will that be a slower scenario case?

          Show
          Lily Wei added a comment - I am not totally understand the performance impact for this issue. However, (280000-190000)/280000 is 32% slower. If we apply that to a big amount of timestamp implementation store procedure. Will that be a slower scenario case?
          Hide
          Kathey Marsden added a comment -

          I think 190000 timestamps per second seems totally reasonable. # I don't know why it was an issue back then. I remember being surprised, but didn't pursue it in more detail.

          Show
          Kathey Marsden added a comment - I think 190000 timestamps per second seems totally reasonable. # I don't know why it was an issue back then. I remember being surprised, but didn't pursue it in more detail.
          Hide
          Knut Anders Hatlen added a comment -

          I committed the 1c patch to trunk with revision 967000.

          Lily: This class is only used for formatting timestamps when writing messages to derby.log, so it's not part of normal query processing unless statement logging is turned on. And even in the cases where logging is turned on, the 32% slow-down is only for the generation of the timestamp, which is just a small part of the total query cost, so the overall performance impact will probably more like 1% than 30% (depending on the complexity of the query, of course). I think that in environments where performance is critical, statement logging is almost certainly turned off, and that's why I don't worry too much about it.

          Show
          Knut Anders Hatlen added a comment - I committed the 1c patch to trunk with revision 967000. Lily: This class is only used for formatting timestamps when writing messages to derby.log, so it's not part of normal query processing unless statement logging is turned on. And even in the cases where logging is turned on, the 32% slow-down is only for the generation of the timestamp, which is just a small part of the total query cost, so the overall performance impact will probably more like 1% than 30% (depending on the complexity of the query, of course). I think that in environments where performance is critical, statement logging is almost certainly turned off, and that's why I don't worry too much about it.
          Hide
          Knut Anders Hatlen added a comment -

          Out of curiosity, I changed the implementation of CheapDateFormatter.formatDate() to

          public static String formatDate(long time)

          { return new Date(time).toString(); }

          and to

          public static String formatDate(long time)

          { return new Date(time).toGMTString(); }

          The toString() variant (which localizes the timestamp) managed 390000 timestamps per second, whereas the toGMTString() variant managed 500000.

          So it seems like the performance of java.util.Date has improved a lot over these years and it's now way faster than CheapDateFormatter. Perhaps now it's time to remove the CheapDateFormatter class altogether and use java.util.Date instead?

          Show
          Knut Anders Hatlen added a comment - Out of curiosity, I changed the implementation of CheapDateFormatter.formatDate() to public static String formatDate(long time) { return new Date(time).toString(); } and to public static String formatDate(long time) { return new Date(time).toGMTString(); } The toString() variant (which localizes the timestamp) managed 390000 timestamps per second, whereas the toGMTString() variant managed 500000. So it seems like the performance of java.util.Date has improved a lot over these years and it's now way faster than CheapDateFormatter. Perhaps now it's time to remove the CheapDateFormatter class altogether and use java.util.Date instead?
          Hide
          Lily Wei added a comment -

          +1
          Use java.util.Date insteads of CheapDateFormatter. I was googleing the performance issue for java.util.Date. There are some issues before 2008. After 2008, I could not find people report performance issue related to java.util.Date. Also, with 500000 timestamps per second testing result, it looks very promise to me.

          Show
          Lily Wei added a comment - +1 Use java.util.Date insteads of CheapDateFormatter. I was googleing the performance issue for java.util.Date. There are some issues before 2008. After 2008, I could not find people report performance issue related to java.util.Date. Also, with 500000 timestamps per second testing result, it looks very promise to me.
          Hide
          Knut Anders Hatlen added a comment -

          Here's a patch that removes CheapDateFormatter and makes the callers
          use java.util.Date instead. CheapDateFormatter used to convert the
          timestamps to GMT, whereas Date.toString() will use the local time
          zone. Date.toString() also formats the timestamp differently. So,
          where CheapDateFormatter produces a timestamp like

          2010-08-18 11:38:59.755 GMT

          we will now print something like this instead:

          Wed Aug 18 13:38:59 CEST 2010

          I think either one of these two formats works just as well as the
          other for logging purposes, but others may disagree.

          Full list of changes made by the patch:

          M java/engine/org/apache/derby/impl/services/monitor/BaseMonitor.java

          Removed unused imports, including CheapDateFormatter. No actual use of
          CheapDateFormatter in this class.

          M java/engine/org/apache/derby/impl/services/locks/Timeout.java

          Use Date.toString() instead of CheapDateFormatter to format the
          timestamp included in timeout exceptions when
          derby.locks.deadlockTrace has been set.

          M java/engine/org/apache/derby/impl/services/stream/BasicGetLogHeader.java

          Use Date.toString() to produce the timestamp in the log stream header.

          M java/engine/org/apache/derby/impl/store/raw/data/BaseDataFileFactory.java

          Use Date.toString() instead of CheapDateFormatter when writing the
          startup and shutdown messages to derby.log. Also removed unused
          imports, including CheapDateFormatter.

          M java/drda/org/apache/derby/impl/drda/NetworkServerControlImpl.java

          Use Date instead of CheapDateFormatter when server prints console
          messages.

          D java/engine/org/apache/derby/iapi/util/CheapDateFormatter.java
          D java/testing/org/apache/derbyTesting/unitTests/junit/CheapDateFormatterTest.java
          M java/testing/org/apache/derbyTesting/unitTests/junit/_Suite.java

          Removed CheapDateFormatter class and its unit tests.

          A java/testing/org/apache/derbyTesting/functionTests/tests/tools/derbyrunjartest_sed.properties
          M java/testing/org/apache/derbyTesting/functionTests/master/derbyrunjartest.out

          Filter out a line with a timestamp from derbyrunjartest since the
          format of the timestamp has changed and isn't caught by the old
          filters.

          Show
          Knut Anders Hatlen added a comment - Here's a patch that removes CheapDateFormatter and makes the callers use java.util.Date instead. CheapDateFormatter used to convert the timestamps to GMT, whereas Date.toString() will use the local time zone. Date.toString() also formats the timestamp differently. So, where CheapDateFormatter produces a timestamp like 2010-08-18 11:38:59.755 GMT we will now print something like this instead: Wed Aug 18 13:38:59 CEST 2010 I think either one of these two formats works just as well as the other for logging purposes, but others may disagree. Full list of changes made by the patch: M java/engine/org/apache/derby/impl/services/monitor/BaseMonitor.java Removed unused imports, including CheapDateFormatter. No actual use of CheapDateFormatter in this class. M java/engine/org/apache/derby/impl/services/locks/Timeout.java Use Date.toString() instead of CheapDateFormatter to format the timestamp included in timeout exceptions when derby.locks.deadlockTrace has been set. M java/engine/org/apache/derby/impl/services/stream/BasicGetLogHeader.java Use Date.toString() to produce the timestamp in the log stream header. M java/engine/org/apache/derby/impl/store/raw/data/BaseDataFileFactory.java Use Date.toString() instead of CheapDateFormatter when writing the startup and shutdown messages to derby.log. Also removed unused imports, including CheapDateFormatter. M java/drda/org/apache/derby/impl/drda/NetworkServerControlImpl.java Use Date instead of CheapDateFormatter when server prints console messages. D java/engine/org/apache/derby/iapi/util/CheapDateFormatter.java D java/testing/org/apache/derbyTesting/unitTests/junit/CheapDateFormatterTest.java M java/testing/org/apache/derbyTesting/unitTests/junit/_Suite.java Removed CheapDateFormatter class and its unit tests. A java/testing/org/apache/derbyTesting/functionTests/tests/tools/derbyrunjartest_sed.properties M java/testing/org/apache/derbyTesting/functionTests/master/derbyrunjartest.out Filter out a line with a timestamp from derbyrunjartest since the format of the timestamp has changed and isn't caught by the old filters.
          Hide
          Knut Anders Hatlen added a comment -

          Committed revision 988874.

          Show
          Knut Anders Hatlen added a comment - Committed revision 988874.

            People

            • Assignee:
              Knut Anders Hatlen
              Reporter:
              Knut Anders Hatlen
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development