Lucene - Core
  1. Lucene - Core
  2. LUCENE-5757

Move RAMUsageEstimator "reflector" to test-framework

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.9, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      This method is "banned" already because it can crawl slowly across large object graphs.

      I think it should be in the test framework. It needs enhancements for testing such as some way to filter what is crawled.

      Currently lots of code in lucene tries to provide accounting information but its all totally untested. We need to fix the test situation or remove Accountable and such methods completely.

      1. LUCENE-5757.patch
        60 kB
        Robert Muir
      2. LUCENE-5757.patch
        53 kB
        Robert Muir

        Activity

        Hide
        Robert Muir added a comment -

        I think this method should be test-framework/o.a.l.util/RamUsageCalculator.java

        So tests can compare their estimates against it, and we can remove the forbidden apis ban (there would be no more super-sneaky method overloading where one of the signatures is slow and the rest is fast).

        Show
        Robert Muir added a comment - I think this method should be test-framework/o.a.l.util/RamUsageCalculator.java So tests can compare their estimates against it, and we can remove the forbidden apis ban (there would be no more super-sneaky method overloading where one of the signatures is slow and the rest is fast).
        Hide
        Uwe Schindler added a comment -

        +1 to move this great tool as a testing tool.

        Show
        Uwe Schindler added a comment - +1 to move this great tool as a testing tool.
        Hide
        Dawid Weiss added a comment -

        RamUsageCalculator: I'd actually prefer to stick to RamUsageEstimator since it really better reflects what it really does.

        Show
        Dawid Weiss added a comment - RamUsageCalculator: I'd actually prefer to stick to RamUsageEstimator since it really better reflects what it really does.
        Hide
        Robert Muir added a comment -

        But then we'd have the same class name in both places.

        I think the reflection part to be moved to tests should have a separate class name than the other stuff that we are keeping in lucene/core (which is mostly constants and simple math).

        Show
        Robert Muir added a comment - But then we'd have the same class name in both places. I think the reflection part to be moved to tests should have a separate class name than the other stuff that we are keeping in lucene/core (which is mostly constants and simple math).
        Hide
        Dawid Weiss added a comment -

        Ah, ok. I didn't get you, I thought you want to move everything.

        Show
        Dawid Weiss added a comment - Ah, ok. I didn't get you, I thought you want to move everything.
        Hide
        Robert Muir added a comment -

        No lots of stuff is using RamUsageEstimator helper methods and constants, its just that one of them is a crawler.

        Currently we banned the crawler method with forbidden-apis for speed and infinite-crawl reasons, but instead i'd rather use it as a test method, and e.g. maybe add callback or filter parameter to it to limit what it crawls (just gear the whole api at testing).

        This way we can test all these rambytesused methods in lucene, which today are completely untested.

        Show
        Robert Muir added a comment - No lots of stuff is using RamUsageEstimator helper methods and constants, its just that one of them is a crawler. Currently we banned the crawler method with forbidden-apis for speed and infinite-crawl reasons, but instead i'd rather use it as a test method, and e.g. maybe add callback or filter parameter to it to limit what it crawls (just gear the whole api at testing). This way we can test all these rambytesused methods in lucene, which today are completely untested.
        Hide
        Robert Muir added a comment -

        Heres a patch, I fixed everything modulo MemoryIndex.

        I named the new class in test-framework RamUsageTester.

        Show
        Robert Muir added a comment - Heres a patch, I fixed everything modulo MemoryIndex. I named the new class in test-framework RamUsageTester.
        Hide
        Robert Muir added a comment -

        Here's a committable patch. I removed the forbidden "rue" stuff (as its only now accessible by tests), and removed the toString() stuff in MemoryIndex (this was a real performance trap, and this is a one-document index...). If for some strange reason someone needs this info on a 1-document index, they can get it from https://github.com/dweiss/java-sizeof or similar.

        Once we do this, we can look at adding features to RamUsageTester so we can better test with it.

        Show
        Robert Muir added a comment - Here's a committable patch. I removed the forbidden "rue" stuff (as its only now accessible by tests), and removed the toString() stuff in MemoryIndex (this was a real performance trap, and this is a one-document index...). If for some strange reason someone needs this info on a 1-document index, they can get it from https://github.com/dweiss/java-sizeof or similar. Once we do this, we can look at adding features to RamUsageTester so we can better test with it.
        Hide
        Michael McCandless added a comment -

        +1

        Show
        Michael McCandless added a comment - +1
        Hide
        Uwe Schindler added a comment -

        Looks fine. I am not yet happy about the class name... If I have a better name I will come back.

        Show
        Uwe Schindler added a comment - Looks fine. I am not yet happy about the class name... If I have a better name I will come back.
        Hide
        Robert Muir added a comment -

        OK, well I'd like to commit this so we can look at improving the testing for these various ram calculations everywhere.

        If you want to rename it from RamUsageTester (i feel this cleanly describes how it should be used), just rename it. its tests.

        Show
        Robert Muir added a comment - OK, well I'd like to commit this so we can look at improving the testing for these various ram calculations everywhere. If you want to rename it from RamUsageTester (i feel this cleanly describes how it should be used), just rename it. its tests.
        Hide
        ASF subversion and git services added a comment -

        Commit 1602515 from Robert Muir in branch 'dev/trunk'
        [ https://svn.apache.org/r1602515 ]

        LUCENE-5757: move RamUsageEstimator reflector to test-framework

        Show
        ASF subversion and git services added a comment - Commit 1602515 from Robert Muir in branch 'dev/trunk' [ https://svn.apache.org/r1602515 ] LUCENE-5757 : move RamUsageEstimator reflector to test-framework
        Hide
        ASF subversion and git services added a comment -

        Commit 1602526 from Robert Muir in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1602526 ]

        LUCENE-5757: move RamUsageEstimator reflector to test-framework

        Show
        ASF subversion and git services added a comment - Commit 1602526 from Robert Muir in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1602526 ] LUCENE-5757 : move RamUsageEstimator reflector to test-framework
        Hide
        ASF subversion and git services added a comment -

        Commit 1607522 from Steve Rowe in branch 'dev/trunk'
        [ https://svn.apache.org/r1607522 ]

        LUCENE-5757: Maven build: forbidden-apis should no longer use the rue.txt signature file

        Show
        ASF subversion and git services added a comment - Commit 1607522 from Steve Rowe in branch 'dev/trunk' [ https://svn.apache.org/r1607522 ] LUCENE-5757 : Maven build: forbidden-apis should no longer use the rue.txt signature file
        Hide
        ASF subversion and git services added a comment -

        Commit 1607523 from Steve Rowe in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1607523 ]

        LUCENE-5757: Maven build: forbidden-apis should no longer use the rue.txt signature file (merged trunk r1607522)

        Show
        ASF subversion and git services added a comment - Commit 1607523 from Steve Rowe in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1607523 ] LUCENE-5757 : Maven build: forbidden-apis should no longer use the rue.txt signature file (merged trunk r1607522)

          People

          • Assignee:
            Unassigned
            Reporter:
            Robert Muir
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development