Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8066291

ZoneIdPrinterParser can be optimized

    Details

    • Subcomponent:
    • Introduced In Version:
      8
    • Resolved In Build:
      b147

      Description

      Every time you parse a ZonedDateTime that contains a zone id the method ZoneRulesProvider.getAvailableZoneIds() is called in the ZoneIdPrinterParser.

      This method can be very costly. Since there is already a cache to avoid to recreate the SubstringTree it should be possible
      to call the ZoneRulesProvider.getAvailableZoneIds() method only when this cache is updated.

      For example something like

      Set regionIds = cachedRegionIds;
      final int regionIdsSize = ZoneRulesProvider.getAvailableZoneIdsSize(); // Return only the size of ZONES in ZoneRulesProvider
      Entry cached = cachedSubstringTree;
      if (cached == null || cached.getKey() != regionIdsSize) {
      cachedRegionIds = ZoneRulesProvider.getAvailableZoneIds();
      ...

      Instead of

      Set regionIds = ZoneRulesProvider.getAvailableZoneIds();
      final int regionIdsSize = regionIds.size();
      Entry cached = cachedSubstringTree;
      if (cached == null || cached.getKey() != regionIdsSize) {
      ...



      Originally proposed as https://github.com/ThreeTen/threeten/issues/350

        Activity

        Hide
        smarks Stuart Marks added a comment -
        There is some discussion of this bug, or at least a related issue, on Stack Overflow:

        http://stackoverflow.com/questions/34374464/extremely-slow-parsing-of-time-zone-with-the-new-java-time-api
        Show
        smarks Stuart Marks added a comment - There is some discussion of this bug, or at least a related issue, on Stack Overflow: http://stackoverflow.com/questions/34374464/extremely-slow-parsing-of-time-zone-with-the-new-java-time-api
        Hide
        scolebourne Stephen Colebourne added a comment -
        The proposed fix seems simple and obvious to me.
        Show
        scolebourne Stephen Colebourne added a comment - The proposed fix seems simple and obvious to me.
        Hide
        bgopularam Bhanu Prakash Gopularam added a comment -
        The ZoneRulesProvider class do not have direct way to get size of ZONES or ZoneIds.
        To implement the suggested solution, we need to add a new public method getAvailableZoneIdsSize() to ZoneRulesProvider which would return number of entries in ZONES list.
        Show
        bgopularam Bhanu Prakash Gopularam added a comment - The ZoneRulesProvider class do not have direct way to get size of ZONES or ZoneIds. To implement the suggested solution, we need to add a new public method getAvailableZoneIdsSize() to ZoneRulesProvider which would return number of entries in ZONES list.
        Hide
        scolebourne Stephen Colebourne added a comment -
        Exactly right. One new static method on ZoneRulesProvider:

        public getAvailableZoneIdsSize() {
         return ZONES.size();
        }

        (located just after getAvailableZoneIds()in the source file)

        Then change the two uses in ZoneIdPrinterParser and ZoneTextPrinterParser to use the new method.
        Show
        scolebourne Stephen Colebourne added a comment - Exactly right. One new static method on ZoneRulesProvider: public getAvailableZoneIdsSize() {  return ZONES.size(); } (located just after getAvailableZoneIds()in the source file) Then change the two uses in ZoneIdPrinterParser and ZoneTextPrinterParser to use the new method.
        Hide
        rriggs Roger Riggs added a comment -
        It would best not to introduce this public method just for an implementation optimization.
        There is no other good use for it in the public API.

        It would be more practical to return a non-Modifiable set of the availableZoneIds that would avoid the copy and for callers that did not need to modify it would be a better choice.
        Show
        rriggs Roger Riggs added a comment - It would best not to introduce this public method just for an implementation optimization. There is no other good use for it in the public API. It would be more practical to return a non-Modifiable set of the availableZoneIds that would avoid the copy and for callers that did not need to modify it would be a better choice.
        Hide
        scolebourne Stephen Colebourne added a comment -
        The method has "@return a modifiable copy of the set of zone IDs, not null"

        Changing from modifiable to immutable is incompatible. I personally have no problem with making that change, and I doubt it will affect many people. Otherwise, the extra method is unfortunately necessary.
        Show
        scolebourne Stephen Colebourne added a comment - The method has "@return a modifiable copy of the set of zone IDs, not null" Changing from modifiable to immutable is incompatible. I personally have no problem with making that change, and I doubt it will affect many people. Otherwise, the extra method is unfortunately necessary.
        Show
        ntv Nadeesh Tv (Inactive) added a comment - http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-May/040842.html
        Hide
        hgupdate HG Updates added a comment -
        URL: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/b09d972a04da
        User: ntv
        Date: 2016-11-21 06:05:24 +0000
        Show
        hgupdate HG Updates added a comment - URL: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/b09d972a04da User: ntv Date: 2016-11-21 06:05:24 +0000
        Hide
        hgupdate HG Updates added a comment -
        URL: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/b09d972a04da
        User: lana
        Date: 2016-11-30 21:39:08 +0000
        Show
        hgupdate HG Updates added a comment - URL: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/b09d972a04da User: lana Date: 2016-11-30 21:39:08 +0000

          People

          • Assignee:
            bgopularam Bhanu Prakash Gopularam
            Reporter:
            rriggs Roger Riggs
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: