Lucene - Core
  1. Lucene - Core
  2. LUCENE-1653

Change DateTools to not create a Calendar in every call to dateToString or timeToString

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.9
    • Component/s: core/other
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      DateTools creates a Calendar instance on every call to dateToString and timeToString. Specifically:

      1. timeToString calls Calendar.getInstance on every call.
      2. dateToString calls timeToString(date.getTime()), which then instantiates a new Date(). I think we should change the order of the calls, or not have each call the other.
      3. round(), which is called from timeToString (after creating a Calendar instance) creates another Calendar instance ...

      Seems that if we synchronize the methods and create the Calendar instance once (static), it should solve it.

      1. cleanerDateTools.patch
        11 kB
        David Smiley
      2. LUCENE-1653.patch
        9 kB
        Mark Miller
      3. LUCENE-1653.patch
        8 kB
        Shai Erera

        Activity

        Hide
        Shai Erera added a comment -

        Had to mark all methods as synchronized, but I think creating a Calendar on every call has much more overhead. I removed the synchronization on the different DateFormats as a result.

        I ran all the tests that use DateTools and they pass (well ... I didn't really change any logic).

        Show
        Shai Erera added a comment - Had to mark all methods as synchronized, but I think creating a Calendar on every call has much more overhead. I removed the synchronization on the different DateFormats as a result. I ran all the tests that use DateTools and they pass (well ... I didn't really change any logic).
        Hide
        Shai Erera added a comment -

        Don't mean to nag ... but can anybody review (and hopefully then commit) this?

        Show
        Shai Erera added a comment - Don't mean to nag ... but can anybody review (and hopefully then commit) this?
        Hide
        Mark Miller added a comment -

        Why is this synchronized?

        + public static synchronized Date round(Date date, Resolution resolution)

        { return new Date(round(date.getTime(), resolution)); }
        Show
        Mark Miller added a comment - Why is this synchronized? + public static synchronized Date round(Date date, Resolution resolution) { return new Date(round(date.getTime(), resolution)); }
        Hide
        Shai Erera added a comment -

        round(long, Resolution) is synchronized and this one calls it, so I figured it cannot hurt to sync it too (sync on an already obtained lock should not have any overhead, or at least significant one). I can remove the sync if you think it matters.

        Show
        Shai Erera added a comment - round(long, Resolution) is synchronized and this one calls it, so I figured it cannot hurt to sync it too (sync on an already obtained lock should not have any overhead, or at least significant one). I can remove the sync if you think it matters.
        Hide
        Mark Miller added a comment -

        Just asking - it just appeared unnecessary. Whats the reason for it? Its just going to get the lock anyway right? I'd just lean towards leaving it out if it removes something (an extra synchronized keyword) and doesn't make a difference. No big deal, was just curious, nothing I'll argue about.

        • Mark
        Show
        Mark Miller added a comment - Just asking - it just appeared unnecessary. Whats the reason for it? Its just going to get the lock anyway right? I'd just lean towards leaving it out if it removes something (an extra synchronized keyword) and doesn't make a difference. No big deal, was just curious, nothing I'll argue about. Mark
        Hide
        Shai Erera added a comment -

        What I had in mind when I synced it was: (1) consistency of the API - all the methods are synced, so you don't need to ask yourself why is method A synced and B not, and do I need to sync B and (2) if we leave it not synced and in the future its impl changes to not call round(), someone might forget to sync it.

        Anyway, those two are not so important. Since we're both on the fence, and you'll be the one committing it, I'll let you make the call

        If you decide to remove the sync, I can either post a new patch, or you can apply the current one and remove the sync before committing. Again ... your call

        Show
        Shai Erera added a comment - What I had in mind when I synced it was: (1) consistency of the API - all the methods are synced, so you don't need to ask yourself why is method A synced and B not, and do I need to sync B and (2) if we leave it not synced and in the future its impl changes to not call round(), someone might forget to sync it. Anyway, those two are not so important. Since we're both on the fence, and you'll be the one committing it, I'll let you make the call If you decide to remove the sync, I can either post a new patch, or you can apply the current one and remove the sync before committing. Again ... your call
        Hide
        Mark Miller added a comment -

        Looks like a good change to me. Only downside I see is that round was not synced before, but now it is. Seems the benefits outweigh though - doesn't have to make the cal each time, and avoids double cal creation that happened when other methods called round. Anyone else have a comment before I commit?

        Show
        Mark Miller added a comment - Looks like a good change to me. Only downside I see is that round was not synced before, but now it is. Seems the benefits outweigh though - doesn't have to make the cal each time, and avoids double cal creation that happened when other methods called round. Anyone else have a comment before I commit?
        Hide
        Mark Miller added a comment -

        add changes.txt

        Show
        Mark Miller added a comment - add changes.txt
        Hide
        Mark Miller added a comment -

        Thanks Shai!

        Show
        Mark Miller added a comment - Thanks Shai!
        Hide
        David Smiley added a comment -

        I'm looking through DateTools now and can't help but want to clean it up some. One thing I see that is odd is the use of a Calendar in timeToString(long,resolution). The first two lines look like this right now:

        calInstance.setTimeInMillis(round(time, resolution));
        Date date = calInstance.getTime();
        

        Instead, it can simply be:

        Date date = new Date(round(time, resolution));
        

        .

        Secondly... I think a good deal of logic can be cleaned up in the other methods instead of a bunch of if-else statements that is a bad code smell. Most of the logic of 3 of those methods could be put into Resolution and be made tighter.

        Show
        David Smiley added a comment - I'm looking through DateTools now and can't help but want to clean it up some. One thing I see that is odd is the use of a Calendar in timeToString(long,resolution). The first two lines look like this right now: calInstance.setTimeInMillis(round(time, resolution)); Date date = calInstance.getTime(); Instead, it can simply be: Date date = new Date(round(time, resolution)); . Secondly... I think a good deal of logic can be cleaned up in the other methods instead of a bunch of if-else statements that is a bad code smell. Most of the logic of 3 of those methods could be put into Resolution and be made tighter.
        Hide
        Shai Erera added a comment -

        In 3.0 when we move to Java 5, we can make Resolution an enum, and then use a switch statement on passed in Resolution. But performance-wise I don't think it would make such a big difference, as we're already comparing instances, which should be relatively fast.

        How will moving the logic of timeToString, stringToDate and round to Resolution make the code tighter? Resolution would still need to check its instance type in order to execute the right code. Unless we subclass Resolution internally and have each subclass implement just the code section of these 3, that it needs?

        Show
        Shai Erera added a comment - In 3.0 when we move to Java 5, we can make Resolution an enum, and then use a switch statement on passed in Resolution. But performance-wise I don't think it would make such a big difference, as we're already comparing instances, which should be relatively fast. How will moving the logic of timeToString, stringToDate and round to Resolution make the code tighter? Resolution would still need to check its instance type in order to execute the right code. Unless we subclass Resolution internally and have each subclass implement just the code section of these 3, that it needs?
        Hide
        David Smiley added a comment -

        Applying the attached patch shows the improvements to DateTools.java that I think should be done. All logic that does anything at all is moved to instance methods of the inner class Resolution. I argue this is more object-oriented.

        1. In cases where Resolution is an argument to the method, I can simply invoke the appropriate call on the Resolution object. Formerly there was a big branch if/else.
        2. Instead of "synchronized" being used seemingly everywhere, synchronized is used to sync on the object that is not threadsafe, be it a DateFormat or Calendar instance.
        3. Since different DateFormat and Calendar instances are created per-Resolution, there is now less lock contention since threads using different resolutions will not use the same locks.
        4. The old implementation of timeToString rounded the time before formatting it. That's unnecessary since the format only includes the resolution desired.
        5.round() now uses a switch statement that benefits from fall-through (no break).

        Another debatable improvement is putting the resolution instances into an array indexed by format length. This would mean I could remove big-ish switch in lookupResolutionByLength() and avoid the length constants there... but I don't think that's a big deal.

        Show
        David Smiley added a comment - Applying the attached patch shows the improvements to DateTools.java that I think should be done. All logic that does anything at all is moved to instance methods of the inner class Resolution. I argue this is more object-oriented. 1. In cases where Resolution is an argument to the method, I can simply invoke the appropriate call on the Resolution object. Formerly there was a big branch if/else. 2. Instead of "synchronized" being used seemingly everywhere, synchronized is used to sync on the object that is not threadsafe, be it a DateFormat or Calendar instance. 3. Since different DateFormat and Calendar instances are created per-Resolution, there is now less lock contention since threads using different resolutions will not use the same locks. 4. The old implementation of timeToString rounded the time before formatting it. That's unnecessary since the format only includes the resolution desired. 5.round() now uses a switch statement that benefits from fall-through (no break). Another debatable improvement is putting the resolution instances into an array indexed by format length. This would mean I could remove big-ish switch in lookupResolutionByLength() and avoid the length constants there... but I don't think that's a big deal.
        Hide
        Mark Miller added a comment -

        Hey David,

        Thanks a lot for the patch!

        Can you attach it to a new issue though? It helps with tracking in CHANGES.txt and svn.

        Show
        Mark Miller added a comment - Hey David, Thanks a lot for the patch! Can you attach it to a new issue though? It helps with tracking in CHANGES.txt and svn.

          People

          • Assignee:
            Mark Miller
            Reporter:
            Shai Erera
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development