Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-7801

SortedSetDocValuesReaderState should implement Accountable

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 7.0, 6.6
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      This class is used by sorted set facets, and wraps per-dimension MultiDocValues.OrdinalMap which already implement accountable, and it's helpful to know how much heap this structure is taking for computing Lucene's SSDV facets.

      1. LUCENE-7801.patch
        6 kB
        Michael McCandless
      2. LUCENE-7801.patch
        6 kB
        Michael McCandless
      3. LUCENE-7801.patch
        4 kB
        Michael McCandless

        Activity

        Hide
        mikemccand Michael McCandless added a comment -

        Simple patch.

        Show
        mikemccand Michael McCandless added a comment - Simple patch.
        Hide
        rcmuir Robert Muir added a comment -

        Maybe add a simple toString impl too for the components involved here? In general it makes it easier to understand where the RAM is going, at least if you use Accountables.toString() to look at it.

        Instead of adding .values() as the child elements, you may want to also use Accountables namedAccountables method: https://github.com/apache/lucene-solr/blob/master/lucene/core/src/java/org/apache/lucene/util/Accountables.java#L100

        So something like namedAccountables("dim", cachedOrdMaps). This way you can see which dim uses which amount of memory? Look at e.g. docvalues or other places doing per-field stuff for some examples.

        Show
        rcmuir Robert Muir added a comment - Maybe add a simple toString impl too for the components involved here? In general it makes it easier to understand where the RAM is going, at least if you use Accountables.toString() to look at it. Instead of adding .values() as the child elements, you may want to also use Accountables namedAccountables method: https://github.com/apache/lucene-solr/blob/master/lucene/core/src/java/org/apache/lucene/util/Accountables.java#L100 So something like namedAccountables("dim", cachedOrdMaps). This way you can see which dim uses which amount of memory? Look at e.g. docvalues or other places doing per-field stuff for some examples.
        Hide
        mikemccand Michael McCandless added a comment -

        Thanks Robert Muir, that's great feedback, I folded it.

        I also added synchronized (cachedOrdMaps), and a TODO comment.

        Show
        mikemccand Michael McCandless added a comment - Thanks Robert Muir , that's great feedback, I folded it. I also added synchronized (cachedOrdMaps) , and a TODO comment.
        Hide
        rcmuir Robert Muir added a comment -

        ok but i still think i would just pass the map to https://github.com/apache/lucene-solr/blob/master/lucene/core/src/java/org/apache/lucene/util/Accountables.java#L100 rather than doing it one by one in your own loop.

        It should be doing the same thing, only with less annoyances (e.g. field names will be sorted rather than haphazard, etc)

        Show
        rcmuir Robert Muir added a comment - ok but i still think i would just pass the map to https://github.com/apache/lucene-solr/blob/master/lucene/core/src/java/org/apache/lucene/util/Accountables.java#L100 rather than doing it one by one in your own loop. It should be doing the same thing, only with less annoyances (e.g. field names will be sorted rather than haphazard, etc)
        Hide
        mikemccand Michael McCandless added a comment -

        Oh sorry I missed the s. How does this look?

        Note that the field name gets included because it's the key for the only entry in the cachedOrdMaps map, so I passed "DefaultSortedSetDocValuesReaderState" as the prefix.

        Show
        mikemccand Michael McCandless added a comment - Oh sorry I missed the s . How does this look? Note that the field name gets included because it's the key for the only entry in the cachedOrdMaps map, so I passed "DefaultSortedSetDocValuesReaderState" as the prefix.
        Hide
        rcmuir Robert Muir added a comment -

        ok, thats kinda confusing the map only ever holds one thing, but unrelated to the issue. I like the improvements, as long as the debugging info looks good it should work and be safe.

        Show
        rcmuir Robert Muir added a comment - ok, thats kinda confusing the map only ever holds one thing, but unrelated to the issue. I like the improvements, as long as the debugging info looks good it should work and be safe.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 2f6101b2ab9e93173a9764450b5f71935495802e in lucene-solr's branch refs/heads/master from Mike McCandless
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2f6101b ]

        LUCENE-7801: SortedSetDocValuesReaderState now implements Accountable

        Show
        jira-bot ASF subversion and git services added a comment - Commit 2f6101b2ab9e93173a9764450b5f71935495802e in lucene-solr's branch refs/heads/master from Mike McCandless [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2f6101b ] LUCENE-7801 : SortedSetDocValuesReaderState now implements Accountable
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 92da9535b5909f07900188cb930564494878a8c7 in lucene-solr's branch refs/heads/branch_6x from Mike McCandless
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=92da953 ]

        LUCENE-7801: SortedSetDocValuesReaderState now implements Accountable

        Show
        jira-bot ASF subversion and git services added a comment - Commit 92da9535b5909f07900188cb930564494878a8c7 in lucene-solr's branch refs/heads/branch_6x from Mike McCandless [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=92da953 ] LUCENE-7801 : SortedSetDocValuesReaderState now implements Accountable
        Hide
        mikemccand Michael McCandless added a comment -

        Thanks Robert Muir!

        Show
        mikemccand Michael McCandless added a comment - Thanks Robert Muir !

          People

          • Assignee:
            mikemccand Michael McCandless
            Reporter:
            mikemccand Michael McCandless
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development