Pentaho Analysis - Mondrian
  1. Pentaho Analysis - Mondrian
  2. MONDRIAN-639

RolapNamedSetEvaluator anon classes implement Iterable, causing performance regression from 2.4 in FunUtil.count()

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Unknown Unknown
    • Resolution: Fixed
    • Affects Version/s: 3.1 GA, 3.1.1 GA, 3.1.2 GA
    • Component/s: None
    • Labels:
      None
    • Notice:
      When an issue is open, the "Fix Version/s" field conveys a target, not necessarily a commitment. When an issue is closed, the "Fix Version/s" field conveys the version that the issue was fixed in.

      Description

      The "includeEmpty == true" path within FunUtil.count() tests to see if the provided iterable implements Collection. If so, it simply calls the size() method to return the count; otherwise, it iterates through each element of the iterable to get the count.

      In older versions of Mondrian (2.4, for example) the iterable passed in usually implemented List, so actually iterating was rare. However, when RolapNamedSetEvaluator was introduced, the iterable became one of two anonymous classes, both of which implement Iterable rather than Collection. This can result in a lot of unnecessary iterations.

      Since both inner classes use a List internally, there's no reason they can't implement the Collection interface as well. I have a change that does exactly this.

      Sample code that illustrates the issue is below. On my mac mini, it takes 233 seconds to execute as-is, and only 4.5 to execute with the proposed fix:

          public void testIteratingCount() {
              // On my mac mini:
              // takes 233 seconds before bug fixed
              // takes 4.5 seconds after bug fixed
              long start = System.currentTimeMillis();
              Result result = executeQuery(
                  "WITH SET [cjoin] AS " +
                  "crossjoin(customers.members, [store type].[store type].members) " +
                  "MEMBER [Measures].[total_available_count] AS Format(COUNT([cjoin]), \"#####\") " +
                  "SELECT" +
                  "{[cjoin]} ON COLUMNS, " +
                  "{[Measures].[total_available_count]} ON ROWS " +
                  "FROM sales");
              System.out.println("elapsed time: " + (System.currentTimeMillis() - start));
              assertEquals(62442, result.getAxes()[0].getPositions().size());
              assertEquals(1, result.getAxes()[1].getPositions().size());
          }

        Activity

        Hide
        Eric McDermid added a comment -
        As per earlier comment, change list 13124 alters the anonymous iterable classes within RolapNamedSetEvaluator.java so that they implement the Collection interface, allowing FunUtil.count() to use a more optimal path. As with the existing implementation of Iterable, any methods that would allow the collection to be altered throw an UnsupportedOperationException to ensure that they aren't accidentally misused in the future.

        The test method mentioned in the bug has been added to PerformanceTest.java. On my system it takes about 4.5 seconds with the fix, 233 seconds without.
        Show
        Eric McDermid added a comment - As per earlier comment, change list 13124 alters the anonymous iterable classes within RolapNamedSetEvaluator.java so that they implement the Collection interface, allowing FunUtil.count() to use a more optimal path. As with the existing implementation of Iterable, any methods that would allow the collection to be altered throw an UnsupportedOperationException to ensure that they aren't accidentally misused in the future. The test method mentioned in the bug has been added to PerformanceTest.java. On my system it takes about 4.5 seconds with the fix, 233 seconds without.
        Hide
        Julian Hyde added a comment -
        Important issue. Make sure fix gets into 3.1.5.
        Show
        Julian Hyde added a comment - Important issue. Make sure fix gets into 3.1.5.

          People

          • Assignee:
            Triage
            Reporter:
            Eric McDermid
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: