Details

    • Type: Task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 6.0
    • Fix Version/s: 6.0
    • Component/s: general/build
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      If we clean up the 'alignment' calculator in RamUsageEstimator, we can compile core with compact1, and the rest of lucene (except tests) with compact2.

      1. LUCENE-6069.patch
        7 kB
        Uwe Schindler
      2. LUCENE-6069.patch
        9 kB
        Robert Muir
      3. LUCENE-6069.patch
        2 kB
        Uwe Schindler
      4. LUCENE-6069.patch
        9 kB
        Robert Muir
      5. LUCENE-6069.patch
        8 kB
        Robert Muir

        Issue Links

          Activity

          Hide
          thetaphi Uwe Schindler added a comment -

          I can fix RAMUsageEstimator to not hard depend on ManagementFactory.

          Show
          thetaphi Uwe Schindler added a comment - I can fix RAMUsageEstimator to not hard depend on ManagementFactory.
          Hide
          rcmuir Robert Muir added a comment -

          I'm going to work on this. I'll start by simply removing this alignment (no need for reflection).

          Lucene is a search engine, not a ram calculator.

          Show
          rcmuir Robert Muir added a comment - I'm going to work on this. I'll start by simply removing this alignment (no need for reflection). Lucene is a search engine, not a ram calculator.
          Hide
          thetaphi Uwe Schindler added a comment -

          Sorry, I forgot about this issue

          Show
          thetaphi Uwe Schindler added a comment - Sorry, I forgot about this issue
          Hide
          rcmuir Robert Muir added a comment -

          Here is a patch fixing this, its unnecessary. all of lucene is compiled with compact3 by default, with lucene-core only requiring compact1.

          I will cleanup the unsafe usage here in ramusageestimator in another issue, that's also a problem.

          Show
          rcmuir Robert Muir added a comment - Here is a patch fixing this, its unnecessary. all of lucene is compiled with compact3 by default, with lucene-core only requiring compact1. I will cleanup the unsafe usage here in ramusageestimator in another issue, that's also a problem.
          Hide
          rcmuir Robert Muir added a comment -

          Improved patch. its compact2 everywhere, with core/ as compact1.

          Fixes a queryparser test that was using the "wrong" Query (javax.management.Query instead of o.a.l.search.Query)

          I think its ready.

          Show
          rcmuir Robert Muir added a comment - Improved patch. its compact2 everywhere, with core/ as compact1. Fixes a queryparser test that was using the "wrong" Query (javax.management.Query instead of o.a.l.search.Query) I think its ready.
          Hide
          thetaphi Uwe Schindler added a comment -

          I fixed it already, patch coming soon. 2 line change

          Show
          thetaphi Uwe Schindler added a comment - I fixed it already, patch coming soon. 2 line change
          Hide
          rcmuir Robert Muir added a comment -

          The thing is, i dont think we should be using all this unsafe shit, etc in a static block in the start of our code.

          We are a search engine not a ram calculator. This code is risky and unnecessary.

          Show
          rcmuir Robert Muir added a comment - The thing is, i dont think we should be using all this unsafe shit, etc in a static block in the start of our code. We are a search engine not a ram calculator. This code is risky and unnecessary.
          Hide
          thetaphi Uwe Schindler added a comment -

          Patch for ManagementFactory

          Show
          thetaphi Uwe Schindler added a comment - Patch for ManagementFactory
          Hide
          thetaphi Uwe Schindler added a comment -

          This is not involving unsafe

          Show
          thetaphi Uwe Schindler added a comment - This is not involving unsafe
          Hide
          rcmuir Robert Muir added a comment -

          Its true the code in question doesnt call unsafe, but there is other code here calling unsafe (and other hacks), and i think its just silly, and for theoretical cases, and we aren't a ram calculator...

          Show
          rcmuir Robert Muir added a comment - Its true the code in question doesnt call unsafe, but there is other code here calling unsafe (and other hacks), and i think its just silly, and for theoretical cases, and we aren't a ram calculator...
          Hide
          rcmuir Robert Muir added a comment -

          I argue there is zero value to this managementfactory code in question. As the comment says, on hotspot its 8. And the code only works for hotspot!

          Lets keep it clean. Unless there is specific objections, I'd like to commit my patch in a few days.

          Show
          rcmuir Robert Muir added a comment - I argue there is zero value to this managementfactory code in question. As the comment says, on hotspot its 8. And the code only works for hotspot! Lets keep it clean. Unless there is specific objections, I'd like to commit my patch in a few days.
          Hide
          thetaphi Uwe Schindler added a comment -

          I know. But instead of removing functionality can we just stick with my patch - a 2 line change removing the ManagamentFactory dependency? If you run on a mobile phone with compact profile it just assumes "8" - otherwise it works as before.

          Removing that is just another thing, but we already rely on more or less exact calculations for Accountable. This makes a lot of difference for Accountable reports (we dont calculate dynamically by refelction, but we sum up constants everywhere). So we should do our best to get the "constants" as exact as possible.

          In fact your patch was not removing completely, the Enum constant was still kept alive and so on. So lets keep what we have, just make it optional.

          Show
          thetaphi Uwe Schindler added a comment - I know. But instead of removing functionality can we just stick with my patch - a 2 line change removing the ManagamentFactory dependency? If you run on a mobile phone with compact profile it just assumes "8" - otherwise it works as before. Removing that is just another thing, but we already rely on more or less exact calculations for Accountable. This makes a lot of difference for Accountable reports (we dont calculate dynamically by refelction, but we sum up constants everywhere). So we should do our best to get the "constants" as exact as possible. In fact your patch was not removing completely, the Enum constant was still kept alive and so on. So lets keep what we have, just make it optional.
          Hide
          rcmuir Robert Muir added a comment -

          I think we should clean it up more. Sorry, your patch wont even work. You also need to fix the "test" that doesn't actually test anything (it apparently just prints, and its tests are @Ignored)

          Sorry man, i don't mean to be rude, but all this is really crap.

          Show
          rcmuir Robert Muir added a comment - I think we should clean it up more. Sorry, your patch wont even work. You also need to fix the "test" that doesn't actually test anything (it apparently just prints, and its tests are @Ignored) Sorry man, i don't mean to be rude, but all this is really crap.
          Hide
          thetaphi Uwe Schindler added a comment -

          What do you mean with Test? I did not change any test. To really test the optional stuff, I added an AssertionError temporaily to the catch block and ran all tests. Worked as it should. So it did not fail.

          Show
          thetaphi Uwe Schindler added a comment - What do you mean with Test? I did not change any test. To really test the optional stuff, I added an AssertionError temporaily to the catch block and ran all tests. Worked as it should. So it did not fail.
          Hide
          thetaphi Uwe Schindler added a comment -

          Sorry man, i don't mean to be rude, but all this is really crap.

          Why? I maintained the code very carefully with Dawid. Please don't call my code crap, this hurts, really! I agree, it is hard to test something that may not be available, but I will look into it to make a test that has a hard check for this.

          Sorry for beeing to late on this issue, I fixed it in 5 minutes, I am very sorry. But don't let us remove functionality that works and helps for our accounting fetaures. In fact this code does not even use any unsafe APIs this is just a perfectly legal way to get this constant. So I am -1 to remove it, sorry.

          Show
          thetaphi Uwe Schindler added a comment - Sorry man, i don't mean to be rude, but all this is really crap. Why? I maintained the code very carefully with Dawid. Please don't call my code crap, this hurts, really! I agree, it is hard to test something that may not be available, but I will look into it to make a test that has a hard check for this. Sorry for beeing to late on this issue, I fixed it in 5 minutes, I am very sorry. But don't let us remove functionality that works and helps for our accounting fetaures. In fact this code does not even use any unsafe APIs this is just a perfectly legal way to get this constant. So I am -1 to remove it, sorry.
          Hide
          rcmuir Robert Muir added a comment -

          See my patch. its only 9kb. just look at it!
          My patch actually compiles all src and tests with compact1/compact2.

          The broken "Test" of this ramusagestimator, StressRamUsageEstimator, (which is not a test, doesnt assert anything, and is @Ignore by default), its a violation too.

          And just like the relevant code in RamUsageEstimator, its useless!!!!!!

          Show
          rcmuir Robert Muir added a comment - See my patch. its only 9kb. just look at it! My patch actually compiles all src and tests with compact1/compact2. The broken "Test" of this ramusagestimator, StressRamUsageEstimator, (which is not a test, doesnt assert anything, and is @Ignore by default), its a violation too. And just like the relevant code in RamUsageEstimator, its useless!!!!!!
          Hide
          rcmuir Robert Muir added a comment -

          Sorry for beeing to late on this issue, I fixed it in 5 minutes, I am very sorry

          Again, you didnt fix it, its not complete. The fake "test" needs to also be fixed/removed, like my patch.

          In fact this code does not even use any unsafe APIs this is just a perfectly legal way to get this constant. So I am -1 to remove it, sorry.

          So you will prevent lucene from being used on more platforms, when the code in question DOES NOTHING, just becausse you think your code, that DOES NOTHING is awesome?

          Show
          rcmuir Robert Muir added a comment - Sorry for beeing to late on this issue, I fixed it in 5 minutes, I am very sorry Again, you didnt fix it, its not complete. The fake "test" needs to also be fixed/removed, like my patch. In fact this code does not even use any unsafe APIs this is just a perfectly legal way to get this constant. So I am -1 to remove it, sorry. So you will prevent lucene from being used on more platforms, when the code in question DOES NOTHING , just becausse you think your code, that DOES NOTHING is awesome?
          Hide
          thetaphi Uwe Schindler added a comment -

          The broken "Test" of this ramusagestimator, StressRamUsageEstimator, (which is not a test, doesnt assert anything, and is @Ignore by default), its a violation too.
          And just like the relevant code in RamUsageEstimator, its useless!!!!!!

          This is not part of this issue.

          Sorry for all that. I looked at you patch, its perfectly fine. I just want to you to merge my patch for RamUsageEstimator into yours - thats all.

          Show
          thetaphi Uwe Schindler added a comment - The broken "Test" of this ramusagestimator, StressRamUsageEstimator, (which is not a test, doesnt assert anything, and is @Ignore by default), its a violation too. And just like the relevant code in RamUsageEstimator, its useless!!!!!! This is not part of this issue. Sorry for all that. I looked at you patch, its perfectly fine. I just want to you to merge my patch for RamUsageEstimator into yours - thats all.
          Hide
          rcmuir Robert Muir added a comment -

          Its part of the issue! The issue is to support compact profiles with compilation.

          Both RAMUsageEstimator, and its test (which does not actually test anything!) are problematic.

          Show
          rcmuir Robert Muir added a comment - Its part of the issue! The issue is to support compact profiles with compilation. Both RAMUsageEstimator, and its test (which does not actually test anything!) are problematic.
          Hide
          thetaphi Uwe Schindler added a comment -

          So you will prevent lucene from being used on more platforms, when the code in question DOES NOTHING, just becausse you think your code, that DOES NOTHING is awesome?

          It does nothing on some platforms and falls back to defaults. It just tries to get the correct constant. What's your problem with that?
          The broken test is another issue, sorry about that.

          Show
          thetaphi Uwe Schindler added a comment - So you will prevent lucene from being used on more platforms, when the code in question DOES NOTHING, just becausse you think your code, that DOES NOTHING is awesome? It does nothing on some platforms and falls back to defaults. It just tries to get the correct constant. What's your problem with that? The broken test is another issue, sorry about that.
          Hide
          rcmuir Robert Muir added a comment -

          Fine, we can merge the reflection call.

          But when it comes to unsafe (another issue), I will attack this much more aggressively. I will call VOTEs or whatever is necessary to remove those unsafe usages.

          Show
          rcmuir Robert Muir added a comment - Fine, we can merge the reflection call. But when it comes to unsafe (another issue), I will attack this much more aggressively. I will call VOTEs or whatever is necessary to remove those unsafe usages.
          Hide
          rcmuir Robert Muir added a comment -

          The broken test is another issue, sorry about that.

          Just to repeat, its not "another issue" but very much a part of this issue.

          Show
          rcmuir Robert Muir added a comment - The broken test is another issue, sorry about that. Just to repeat, its not "another issue" but very much a part of this issue.
          Hide
          mikemccand Michael McCandless added a comment -

          +1 to compile with compact profiles.

          I think in general Lucene should use as minimal APIs as are truly needed to get indexing and searching done.

          E.g., this same "Occam's razor" philosophy has served us well in pruning back the Directory API over time.

          Also, this motivation is completely separate from claims that this change might help "abusive" use cases, like mobile.

          Show
          mikemccand Michael McCandless added a comment - +1 to compile with compact profiles. I think in general Lucene should use as minimal APIs as are truly needed to get indexing and searching done. E.g., this same "Occam's razor" philosophy has served us well in pruning back the Directory API over time. Also, this motivation is completely separate from claims that this change might help "abusive" use cases, like mobile.
          Hide
          thetaphi Uwe Schindler added a comment -

          +1 To Robert's patch here, with the reflection I provided as separate patch!

          The reflection will be needed for the nuke of Unsafe anyways, because we need HotspotMXBean to detect reference size (which is still important, I just reviewed the code using all parts using the constant from RAMUsageEstimator). If one runs on compact profile on 64 bit without HotspotBean it will just assume that a refenece pointer is 8 bytes. If its uses compressed Oops we just calculate the size of arrays wrong (by factor of 2).

          In my opinion: We should have good constants for huge systems, because with heap sizes around several gigabytes, the memory reporting by Lucene/Solr/Elasticsearch should not be wrong by factor 2 for some structures. I don't care about static object headers or alignments, if they are wrong - it has less effect (because we generally have few huge objects in FST/Filter/DocValues).

          If you use compact profile on some platform, the ram usage reporting is in most cases not so interesting, because you are already limited by the platform...

          Show
          thetaphi Uwe Schindler added a comment - +1 To Robert's patch here, with the reflection I provided as separate patch! The reflection will be needed for the nuke of Unsafe anyways, because we need HotspotMXBean to detect reference size (which is still important, I just reviewed the code using all parts using the constant from RAMUsageEstimator). If one runs on compact profile on 64 bit without HotspotBean it will just assume that a refenece pointer is 8 bytes. If its uses compressed Oops we just calculate the size of arrays wrong (by factor of 2). In my opinion: We should have good constants for huge systems, because with heap sizes around several gigabytes, the memory reporting by Lucene/Solr/Elasticsearch should not be wrong by factor 2 for some structures. I don't care about static object headers or alignments, if they are wrong - it has less effect (because we generally have few huge objects in FST/Filter/DocValues). If you use compact profile on some platform, the ram usage reporting is in most cases not so interesting, because you are already limited by the platform...
          Hide
          rcmuir Robert Muir added a comment -

          Updated patch: this one is a compromise, i use Uwe's changes for RamUsageEstimator + my changes for build and tests.

          tests pass.

          Show
          rcmuir Robert Muir added a comment - Updated patch: this one is a compromise, i use Uwe's changes for RamUsageEstimator + my changes for build and tests. tests pass.
          Hide
          thetaphi Uwe Schindler added a comment -

          Patch with the conflicting changes in RUE removed (already done in LUCENE-6239).

          Show
          thetaphi Uwe Schindler added a comment - Patch with the conflicting changes in RUE removed (already done in LUCENE-6239 ).
          Hide
          rcmuir Robert Muir added a comment -

          +1 to commit!

          Show
          rcmuir Robert Muir added a comment - +1 to commit!
          Hide
          thetaphi Uwe Schindler added a comment -

          +1 to commit in trunk! Maybe add some good CHANGES.txt so its public like "Lucene Core is Compact profile"

          Show
          thetaphi Uwe Schindler added a comment - +1 to commit in trunk! Maybe add some good CHANGES.txt so its public like "Lucene Core is Compact profile"
          Hide
          rcmuir Robert Muir added a comment -

          You can commit, if you want. its is your latest patch. Can we backport the tests changes only to branch_5x?

          Show
          rcmuir Robert Muir added a comment - You can commit, if you want. its is your latest patch. Can we backport the tests changes only to branch_5x?
          Hide
          thetaphi Uwe Schindler added a comment -

          OK, will do this. I will now run the whole build, to validate all is fine with Java 8.

          Show
          thetaphi Uwe Schindler added a comment - OK, will do this. I will now run the whole build, to validate all is fine with Java 8.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1659347 from Uwe Schindler in branch 'dev/trunk'
          [ https://svn.apache.org/r1659347 ]

          LUCENE-6069: Lucene Core now gets compiled with Java 8 "compact1" profile, all other modules with "compact2".

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1659347 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1659347 ] LUCENE-6069 : Lucene Core now gets compiled with Java 8 "compact1" profile, all other modules with "compact2".
          Hide
          thetaphi Uwe Schindler added a comment -

          Thanks Robert!
          I also backported the test changes to get rid of crazy stuff.

          Show
          thetaphi Uwe Schindler added a comment - Thanks Robert! I also backported the test changes to get rid of crazy stuff.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1659349 from Uwe Schindler in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1659349 ]

          LUCENE-6069: Backport the test changes to 5.x

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1659349 from Uwe Schindler in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1659349 ] LUCENE-6069 : Backport the test changes to 5.x

            People

            • Assignee:
              rcmuir Robert Muir
              Reporter:
              rcmuir Robert Muir
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development