Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.6.1
    • Fix Version/s: 4.4, 6.0
    • Component/s: core/store
    • Labels:
      None
    • Environment:

      Linux debian squeeze 64 bit, Oracle JDK 6, 32 GB RAM, 16 cores
      -Xmx16G

    • Lucene Fields:
      New

      Description

      We are running a set of independent search machines, running our custom software using lucene as a search library. We recently upgraded from lucene 3.0.3 to 3.6.1 and noticed a severe degradation of performance.

      After doing some heap dump digging, it turns out the process is stalling because it's spending so much time in GC. We noticed about 212 million WeakReference, originating from WeakIdentityMap, originating from MMapIndexInput.

      Our problem completely went away after removing the clones weakhashmap from MMapIndexInput, and as a side-effect, disabling support for explictly unmapping the mmapped data.

      1. LUCENE-4740.patch
        6 kB
        Uwe Schindler
      2. LUCENE-4740.patch
        3 kB
        Uwe Schindler

        Issue Links

          Activity

          Hide
          Kristofer Karlsson added a comment -

          I am not sure if this is a problem in practice, but there are also WeakIdentityMaps used in AttributeImpl, AttributeSource, VirtualMethod where the keys of the map are classes, which I don't imagine get allocated or collected a lot at all.

          I replaced those maps with regular ConcurrentHashMap<Class, X> without any negative impact.

          Show
          Kristofer Karlsson added a comment - I am not sure if this is a problem in practice, but there are also WeakIdentityMaps used in AttributeImpl, AttributeSource, VirtualMethod where the keys of the map are classes, which I don't imagine get allocated or collected a lot at all. I replaced those maps with regular ConcurrentHashMap<Class, X> without any negative impact.
          Hide
          Kristofer Karlsson added a comment -

          This seems to be somewhat addressed in trunk, with the inputs in http://svn.apache.org/repos/asf/lucene/dev/trunk/lucene/core/src/java/org/apache/lucene/store/ByteBufferIndexInput.java removing themselves from the clones list on close(). Should that be backported to 3.x?

          Show
          Kristofer Karlsson added a comment - This seems to be somewhat addressed in trunk, with the inputs in http://svn.apache.org/repos/asf/lucene/dev/trunk/lucene/core/src/java/org/apache/lucene/store/ByteBufferIndexInput.java removing themselves from the clones list on close(). Should that be backported to 3.x?
          Hide
          Uwe Schindler added a comment - - edited

          I replaced those maps with regular ConcurrentHashMap<Class, X> without any negative impact.

          This kills the possibility of unloading web applications in application servers. Those maps don't produce many references.

          Which OS are you using? Somebody had a similar problem on Windows - but on Linux all was fine. Maybe some JVM version has a bug in reference queues... Can you give your exact version number?

          Show
          Uwe Schindler added a comment - - edited I replaced those maps with regular ConcurrentHashMap<Class, X> without any negative impact. This kills the possibility of unloading web applications in application servers. Those maps don't produce many references. Which OS are you using? Somebody had a similar problem on Windows - but on Linux all was fine. Maybe some JVM version has a bug in reference queues... Can you give your exact version number?
          Hide
          Kristofer Karlsson added a comment -

          Yes, that change is probably not a good general solution, but it worked well for our usecase. It might be nice to have support for unloadable classes optional.

          I am using debian squeeze, 64 bit, Oracle JDK 6 with a 16 GB heap and four indexes, totalling 9 GB

          java version "1.6.0_26"
          Java(TM) SE Runtime Environment (build 1.6.0_26-b03)
          Java HotSpot(TM) 64-Bit Server VM (build 20.1-b02, mixed mode)
          

          The 212M WeakReferences on the heap indicates that the referencequeues weren't functioning correctly.

          In any case, if the useUnmap is false, then it seems unnecessary to even add references to the clones to the map.

          Show
          Kristofer Karlsson added a comment - Yes, that change is probably not a good general solution, but it worked well for our usecase. It might be nice to have support for unloadable classes optional. I am using debian squeeze, 64 bit, Oracle JDK 6 with a 16 GB heap and four indexes, totalling 9 GB java version "1.6.0_26" Java(TM) SE Runtime Environment (build 1.6.0_26-b03) Java HotSpot(TM) 64-Bit Server VM (build 20.1-b02, mixed mode) The 212M WeakReferences on the heap indicates that the referencequeues weren't functioning correctly. In any case, if the useUnmap is false, then it seems unnecessary to even add references to the clones to the map.
          Hide
          Robert Muir added a comment -

          In any case, if the useUnmap is false, then it seems unnecessary to even add references to the clones to the map.

          +1, I think make it null in this case. it just gives the user options (with different tradeoffs).

          Show
          Robert Muir added a comment - In any case, if the useUnmap is false, then it seems unnecessary to even add references to the clones to the map. +1, I think make it null in this case. it just gives the user options (with different tradeoffs).
          Hide
          Uwe Schindler added a comment -

          Yes, that change is probably not a good general solution, but it worked well for our usecase. It might be nice to have support for unloadable classes optional.

          As I said, a change in AttributeSource or VirtualMethod is not needed, the number of total per-JVM references there are in the number of 10s. This is perfectly fine code and nobody needs to change anything. No need for "optional" class unloading. Not using weak references here would be a major design issue and a large leak.

          In any case, if the useUnmap is false, then it seems unnecessary to even add references to the clones to the map.

          Robert and me were discussing about that already, we can do that, this patch is easy. We can offer that as an option (the no-unmap option), with the backside of e.g. windows can no longer delete index files unless they are garbage collected and especially higher disk usage while indexing.

          I did some testing with various JDKs on windows 64 bit, using a loop that clones one indexinput over and over. This loop runs successful for hours without OOM, so there is no cleanup problem, ReferenceQueues are working correctly. With a heap size of 512 MB and this simple loop, the number of Weak references is between 5000 and 600,000. But indeed, there are some GC pauses (in JDK 6 and 7). The reason for this is: Weak referees are a little bit more "reachable" than unreachable objects, so GC let them survive for a longer time than unreachable ones. There is nothing we can do against that. The main problem in your case maybe the really large heap size: why do you need it?

          My second test was to close every cloned index input (trunk/4.x only, where the commit you mentioned was added by me one week ago), in that case the number of references was of course a static "1" In this test, no GC pauses occurred and the test ran faster.

          In my final test I disabled the put() to the WeakIdentityMap completely, in that case it was again faster, but this was caused more by the complete non-existence of any locking or maintenance of the ConcurrentHashMap.

          The times for 300 million clones:

          • With default Lucene 4.x/trunk, no close of clones (Lucene never closes clones and thats almost impossible to add): 200 secs, GC pauses
          • With closing clones: 65 secs
          • Without any map: 40 secs

          (JDK 6u32, windows, 64 bit, server vm, default garbage collector)

            // for this test, make the clones map in ByteBufferIndexInput public/package-private/...
            public void testGC() throws Exception {
              MMapDirectory mmapDir = new MMapDirectory(_TestUtil.getTempDir("testGC"));
              IndexOutput io = mmapDir.createOutput("bytes", newIOContext(random()));
              io.writeVInt(5);
              io.close();
              IndexInput ii = mmapDir.openInput("bytes", IOContext.DEFAULT);
              int hash = 0;
              for (int i = 0; i < 300*1024*1024; i++) {
                final IndexInput clone = ii.clone();
                hash += System.identityHashCode(clone);
                if (i % (10*1024) == 0) {
                  System.out.println("Number of clones: " + ((ByteBufferIndexInput) ii).clones.size());
                }
                //clone.close();
              }
              ii.close();
              mmapDir.close();
            }
          

          In any case, we can allow user to disable unmap, but we then have to keep the weak references to the clones when unmapping is enabled, unless we add close() of clones to Lucene everywhere...

          Some other ideas are: Reuse the ByteBufferIndexInput instances, so we dont need to recreate them all the time. I have no idea how to do that, because we have no close() to release those, which brings us back to that problem again.

          Show
          Uwe Schindler added a comment - Yes, that change is probably not a good general solution, but it worked well for our usecase. It might be nice to have support for unloadable classes optional. As I said, a change in AttributeSource or VirtualMethod is not needed, the number of total per-JVM references there are in the number of 10s. This is perfectly fine code and nobody needs to change anything. No need for "optional" class unloading. Not using weak references here would be a major design issue and a large leak. In any case, if the useUnmap is false, then it seems unnecessary to even add references to the clones to the map. Robert and me were discussing about that already, we can do that, this patch is easy. We can offer that as an option (the no-unmap option), with the backside of e.g. windows can no longer delete index files unless they are garbage collected and especially higher disk usage while indexing. I did some testing with various JDKs on windows 64 bit, using a loop that clones one indexinput over and over. This loop runs successful for hours without OOM, so there is no cleanup problem, ReferenceQueues are working correctly. With a heap size of 512 MB and this simple loop, the number of Weak references is between 5000 and 600,000. But indeed, there are some GC pauses (in JDK 6 and 7). The reason for this is: Weak referees are a little bit more "reachable" than unreachable objects, so GC let them survive for a longer time than unreachable ones. There is nothing we can do against that. The main problem in your case maybe the really large heap size: why do you need it? My second test was to close every cloned index input (trunk/4.x only, where the commit you mentioned was added by me one week ago), in that case the number of references was of course a static "1" In this test, no GC pauses occurred and the test ran faster. In my final test I disabled the put() to the WeakIdentityMap completely, in that case it was again faster, but this was caused more by the complete non-existence of any locking or maintenance of the ConcurrentHashMap. The times for 300 million clones: With default Lucene 4.x/trunk, no close of clones (Lucene never closes clones and thats almost impossible to add) : 200 secs, GC pauses With closing clones: 65 secs Without any map: 40 secs (JDK 6u32, windows, 64 bit, server vm, default garbage collector) // for this test, make the clones map in ByteBufferIndexInput public / package - private /... public void testGC() throws Exception { MMapDirectory mmapDir = new MMapDirectory(_TestUtil.getTempDir( "testGC" )); IndexOutput io = mmapDir.createOutput( "bytes" , newIOContext(random())); io.writeVInt(5); io.close(); IndexInput ii = mmapDir.openInput( "bytes" , IOContext.DEFAULT); int hash = 0; for ( int i = 0; i < 300*1024*1024; i++) { final IndexInput clone = ii.clone(); hash += System .identityHashCode(clone); if (i % (10*1024) == 0) { System .out.println( " Number of clones: " + ((ByteBufferIndexInput) ii).clones.size()); } //clone.close(); } ii.close(); mmapDir.close(); } In any case, we can allow user to disable unmap, but we then have to keep the weak references to the clones when unmapping is enabled, unless we add close() of clones to Lucene everywhere... Some other ideas are: Reuse the ByteBufferIndexInput instances, so we dont need to recreate them all the time. I have no idea how to do that, because we have no close() to release those, which brings us back to that problem again.
          Hide
          Kristofer Karlsson added a comment -

          Agree that the weakreferences for classes is probably a very minor part of it, and very unlikely part of the problem here.

          The unmap option is nice, and you could make it less complicated by simply disallowing the option to be changed after the mmapdirectory has ever been cloned. In practice it will always be set immediately after construction.

          Show
          Kristofer Karlsson added a comment - Agree that the weakreferences for classes is probably a very minor part of it, and very unlikely part of the problem here. The unmap option is nice, and you could make it less complicated by simply disallowing the option to be changed after the mmapdirectory has ever been cloned. In practice it will always be set immediately after construction.
          Hide
          Kristofer Karlsson added a comment -

          After doing some more thinking and micro-benchmarking, I think the problem occurs when you create clones at a faster rate than the GC can cope with.

          public class WeakTest extends TestCase {
              private static final int CPUS = Runtime.getRuntime().availableProcessors();
          
              public void testFoo() throws Exception {
                  final WeakIdentityMap<Object, Boolean> map = WeakIdentityMap.newConcurrentHashMap();
                  ExecutorService executorService = Executors.newCachedThreadPool();
          
                  for (int k = 0; k < CPUS; k++) {
                      executorService.submit(new Runnable() {
                          public void run() {
                              while (true) {
                                  map.put(new Object(), Boolean.TRUE);
                              }
                          }
                      });
                      executorService.submit(new Runnable() {
                          public void run() {
                              while (true) {
                                  System.gc();
                              }
                          }
                      });
                  }
                  while (true) {
                      System.out.println("Map size: " + map.size());
                      Thread.sleep(1000);
                  }
              }
          
          }
          

          I tried running this with -Xmx100m. This makes the map grow indefinitely.
          I know this is not a very reliable test, since the JVM only considers System.gc() a hint, but it definitely seems like it's not very good at freeing weak references during high load.

          If I add Thread.sleep(1) in the put-worker, I can make it grow slower, but it still seems to grow over time.

          (Running java 7 on my home computer)

          java version "1.7.0_07"
          Java(TM) SE Runtime Environment (build 1.7.0_07-b10)
          Java HotSpot(TM) 64-Bit Server VM (build 23.3-b01, mixed mode)
          
          Show
          Kristofer Karlsson added a comment - After doing some more thinking and micro-benchmarking, I think the problem occurs when you create clones at a faster rate than the GC can cope with. public class WeakTest extends TestCase { private static final int CPUS = Runtime.getRuntime().availableProcessors(); public void testFoo() throws Exception { final WeakIdentityMap<Object, Boolean> map = WeakIdentityMap.newConcurrentHashMap(); ExecutorService executorService = Executors.newCachedThreadPool(); for (int k = 0; k < CPUS; k++) { executorService.submit(new Runnable() { public void run() { while (true) { map.put(new Object(), Boolean.TRUE); } } }); executorService.submit(new Runnable() { public void run() { while (true) { System.gc(); } } }); } while (true) { System.out.println("Map size: " + map.size()); Thread.sleep(1000); } } } I tried running this with -Xmx100m. This makes the map grow indefinitely. I know this is not a very reliable test, since the JVM only considers System.gc() a hint, but it definitely seems like it's not very good at freeing weak references during high load. If I add Thread.sleep(1) in the put-worker, I can make it grow slower, but it still seems to grow over time. (Running java 7 on my home computer) java version "1.7.0_07" Java(TM) SE Runtime Environment (build 1.7.0_07-b10) Java HotSpot(TM) 64-Bit Server VM (build 23.3-b01, mixed mode)
          Hide
          Uwe Schindler added a comment -

          Attached you will find the patch, that disables tracking of clones if the unmapping is disabled. We dont need to make the setting in MMapDirectory unmodifiable, after changing it all IndexInputs created afterwards, the new setting is used. This does not differ from the previous behaviour or unmapping at all.

          In general, people should in any case set this setting after constrcution (we may add a ctor param, too).

          Show
          Uwe Schindler added a comment - Attached you will find the patch, that disables tracking of clones if the unmapping is disabled. We dont need to make the setting in MMapDirectory unmodifiable, after changing it all IndexInputs created afterwards, the new setting is used. This does not differ from the previous behaviour or unmapping at all. In general, people should in any case set this setting after constrcution (we may add a ctor param, too).
          Hide
          Uwe Schindler added a comment -

          After doing some more thinking and micro-benchmarking, I think the problem occurs when you create clones at a faster rate than the GC can cope with.

          I agree that might be aproblem and you may be facing it. How mayn requests per second do you have on your server?

          This behaviour is Java's weak reference GC behaviour, it has nothing to do with WeakIdentityMap. The default WeakHashMap from JDK has the same problems.

          Agree that the weakreferences for classes is probably a very minor part of it, and very unlikely part of the problem here.

          That is very common, the JDK uses the same mechanism like in AttributeSource at several places. It is definitely not part of the problem.

          The problem here is the weak map that has a very high throughput of puts (every query produces at least one IndexInput clone, possibly more). The high throughput already lead to the change to WeakIdentityMap recently, because a synchronized WeakHashMap was not able to handle the large number of concurrent puts (Lucene 3.6.0 regression).

          I am currently thinking of making the whole thing work without weak references and instead have some "hard reference" from the clone to master (it is already there, MappedByteBuffer.duplicate() returns a duplicate buffer that has a reference to the master). The problem with this is, that you need a check on every access of the IndexInput if the buffer is still valid. If it is only some null check, we may add it, but its risky for performance too.

          My ide was that the master creates some boolean[1] and passes this boolen[1] array to all childs. When the master closes, it does set the b[0] to false. All childs would do a check on b[0]... Not sure how this affects performance.

          Show
          Uwe Schindler added a comment - After doing some more thinking and micro-benchmarking, I think the problem occurs when you create clones at a faster rate than the GC can cope with. I agree that might be aproblem and you may be facing it. How mayn requests per second do you have on your server? This behaviour is Java's weak reference GC behaviour, it has nothing to do with WeakIdentityMap. The default WeakHashMap from JDK has the same problems. Agree that the weakreferences for classes is probably a very minor part of it, and very unlikely part of the problem here. That is very common, the JDK uses the same mechanism like in AttributeSource at several places. It is definitely not part of the problem. The problem here is the weak map that has a very high throughput of puts (every query produces at least one IndexInput clone, possibly more). The high throughput already lead to the change to WeakIdentityMap recently, because a synchronized WeakHashMap was not able to handle the large number of concurrent puts (Lucene 3.6.0 regression). I am currently thinking of making the whole thing work without weak references and instead have some "hard reference" from the clone to master (it is already there, MappedByteBuffer.duplicate() returns a duplicate buffer that has a reference to the master). The problem with this is, that you need a check on every access of the IndexInput if the buffer is still valid. If it is only some null check, we may add it, but its risky for performance too. My ide was that the master creates some boolean [1] and passes this boolen [1] array to all childs. When the master closes, it does set the b [0] to false. All childs would do a check on b [0] ... Not sure how this affects performance.
          Hide
          Kristofer Karlsson added a comment -

          Looks good, but what happens if you start with having useUnmap = false, then creating a bunch of clones, and then setting it back to useUnmap = true?

          If I read the code correctly (which I am not certain of), closing the original input will then unmap the data and break all the existing clones.

          Show
          Kristofer Karlsson added a comment - Looks good, but what happens if you start with having useUnmap = false, then creating a bunch of clones, and then setting it back to useUnmap = true? If I read the code correctly (which I am not certain of), closing the original input will then unmap the data and break all the existing clones.
          Hide
          Kristofer Karlsson added a comment -

          I agree that might be aproblem and you may be facing it. How mayn requests per second do you have on your server?

          Not that many - about 8000 per minute on yesterdays peak, which is about 133 per second. However, each requests leads to several complex lucene queries, though I don't have any numbers on the actual lucene query throughput.

          This behaviour is Java's weak reference GC behaviour, it has nothing to do with WeakIdentityMap. The default WeakHashMap from JDK has the same problems.

          Agreed.

          My ide was that the master creates some boolean[1] and passes this boolen[1] array to all childs. When the master closes, it does set the b[0] to false. All childs would do a check on b[0]... Not sure how this affects performance.

          Yes, I thought about this too, and I am not sure the performance penalty would be that problematic (but it would need to be measured). And if possibly, users of the inputs should avoid doing small individual byte gets, and instead try to consume chunks of bytes to avoid the overhead.

          I would prefer an AtomicBoolean since it uses a volatile field. As far as I know, you can't make contents of arrays volatile.
          In any case, wouldn't it would possible to skip the whole master/slave relationship and make everyone equal, just sharing the closed state flag? Though then running close() on a clone would close everything, which is possibly not what you want to happen.

          Show
          Kristofer Karlsson added a comment - I agree that might be aproblem and you may be facing it. How mayn requests per second do you have on your server? Not that many - about 8000 per minute on yesterdays peak, which is about 133 per second. However, each requests leads to several complex lucene queries, though I don't have any numbers on the actual lucene query throughput. This behaviour is Java's weak reference GC behaviour, it has nothing to do with WeakIdentityMap. The default WeakHashMap from JDK has the same problems. Agreed. My ide was that the master creates some boolean [1] and passes this boolen [1] array to all childs. When the master closes, it does set the b [0] to false. All childs would do a check on b [0] ... Not sure how this affects performance. Yes, I thought about this too, and I am not sure the performance penalty would be that problematic (but it would need to be measured). And if possibly, users of the inputs should avoid doing small individual byte gets, and instead try to consume chunks of bytes to avoid the overhead. I would prefer an AtomicBoolean since it uses a volatile field. As far as I know, you can't make contents of arrays volatile. In any case, wouldn't it would possible to skip the whole master/slave relationship and make everyone equal, just sharing the closed state flag? Though then running close() on a clone would close everything, which is possibly not what you want to happen.
          Hide
          Uwe Schindler added a comment -

          but what happens if you start with having useUnmap = false, then creating a bunch of clones, and then setting it back to useUnmap = true? If I read the code correctly (which I am not certain of), closing the original input will then unmap the data and break all the existing clones.

          The settings are decoupled:
          If you start with useUnmap = false, all IndexInputs created will have no weak map, so when they are closed, the clones are not touched.

          If you change the setting to true, the already existing indexinputs will not be tracked (as before), but new indexinputs will get a map and all of their clones will be freed correctly.

          The other special case: If you change the setting from true -> false, all existing IndexInputs will keep their maps and will be cleaned up on close (buffers set to null). But the cleanMapping() method will get a no-op, so they are correctly nulled, but no longer unmapped.

          In any case a SIGSEGV is prevented (as good as we can without locking).

          In general, nothing breaks if you change the setting later, but you should really do it only after construction.

          Show
          Uwe Schindler added a comment - but what happens if you start with having useUnmap = false, then creating a bunch of clones, and then setting it back to useUnmap = true? If I read the code correctly (which I am not certain of), closing the original input will then unmap the data and break all the existing clones. The settings are decoupled: If you start with useUnmap = false, all IndexInputs created will have no weak map, so when they are closed, the clones are not touched. If you change the setting to true, the already existing indexinputs will not be tracked (as before), but new indexinputs will get a map and all of their clones will be freed correctly. The other special case: If you change the setting from true -> false, all existing IndexInputs will keep their maps and will be cleaned up on close (buffers set to null). But the cleanMapping() method will get a no-op, so they are correctly nulled, but no longer unmapped. In any case a SIGSEGV is prevented (as good as we can without locking). In general, nothing breaks if you change the setting later, but you should really do it only after construction.
          Hide
          Uwe Schindler added a comment -

          I would prefer an AtomicBoolean since it uses a volatile field. As far as I know, you can't make contents of arrays volatile.

          This kills performance. MMapIndexInput would be slower than SimpleFSIndexInput! This is why the array is used as a fake "reference" to a boolean.

          The current approach of unmapping the byte buffers and protecting for sigsegv by nulling them is not 100% safe. The JVM may still crash if another thread does not yet see the nulled buffer. But in most cases the use will get a AlreadyClosedException and can fix his code before he goes into production and his JVM crashes suddenly.

          Show
          Uwe Schindler added a comment - I would prefer an AtomicBoolean since it uses a volatile field. As far as I know, you can't make contents of arrays volatile. This kills performance. MMapIndexInput would be slower than SimpleFSIndexInput! This is why the array is used as a fake "reference" to a boolean. The current approach of unmapping the byte buffers and protecting for sigsegv by nulling them is not 100% safe. The JVM may still crash if another thread does not yet see the nulled buffer. But in most cases the use will get a AlreadyClosedException and can fix his code before he goes into production and his JVM crashes suddenly.
          Hide
          Kristofer Karlsson added a comment -

          If you change the setting to true, the already existing indexinputs will not be tracked (as before), but new indexinputs will get a map and all of their clones will be freed correctly.

          Right, but the already existing indexinputs will have buffers pointing to the same bytebuffer, so if you close the master, you would get SIGSEGV in the clones, since the master can not forcibly close the clones.

          Show
          Kristofer Karlsson added a comment - If you change the setting to true, the already existing indexinputs will not be tracked (as before), but new indexinputs will get a map and all of their clones will be freed correctly. Right, but the already existing indexinputs will have buffers pointing to the same bytebuffer, so if you close the master, you would get SIGSEGV in the clones, since the master can not forcibly close the clones.
          Hide
          Kristofer Karlsson added a comment -

          freeBuffers in MMapIndexInput only looks at MMapDirectory.useUnmap, which is the thing that may change, unlike the trackClones / clones which is fixed once the master has been created.

          I propose adding this to close():

          if (clones != null) { freeBuffers(); }
          
          Show
          Kristofer Karlsson added a comment - freeBuffers in MMapIndexInput only looks at MMapDirectory.useUnmap, which is the thing that may change, unlike the trackClones / clones which is fixed once the master has been created. I propose adding this to close(): if (clones != null) { freeBuffers(); }
          Hide
          Uwe Schindler added a comment -

          Right, but the already existing indexinputs will have buffers pointing to the same bytebuffer, so if you close the master, you would get SIGSEGV in the clones, since the master can not forcibly close the clones.

          Right that can happen, the fix is to make the freeBuffer method use the setting of the actual IndexInput, not the global one of MMapDirectory.

          Show
          Uwe Schindler added a comment - Right, but the already existing indexinputs will have buffers pointing to the same bytebuffer, so if you close the master, you would get SIGSEGV in the clones, since the master can not forcibly close the clones. Right that can happen, the fix is to make the freeBuffer method use the setting of the actual IndexInput, not the global one of MMapDirectory.
          Hide
          Uwe Schindler added a comment -

          I propose adding this to close():

          thats not good and makes reuse of ByteBufferIndexInput complicated. MMapDirectory have to take care in its overridden abstract method (by using the IndexInput's instance setting.

          Show
          Uwe Schindler added a comment - I propose adding this to close(): thats not good and makes reuse of ByteBufferIndexInput complicated. MMapDirectory have to take care in its overridden abstract method (by using the IndexInput's instance setting.
          Hide
          Uwe Schindler added a comment -

          Patch that makes the unmapping behaviour consistent. The "unmap" setting is now maintained by each MMapIndexInput. The unmap method was also moved into the impl class (the previous implementation was a relict from older times)

          Show
          Uwe Schindler added a comment - Patch that makes the unmapping behaviour consistent. The "unmap" setting is now maintained by each MMapIndexInput. The unmap method was also moved into the impl class (the previous implementation was a relict from older times)
          Hide
          Uwe Schindler added a comment -

          Better patch using the getter method instead stupid MMapDirectory.this.useUnmap.

          Show
          Uwe Schindler added a comment - Better patch using the getter method instead stupid MMapDirectory.this.useUnmap.
          Hide
          Kristofer Karlsson added a comment -

          That looks good to me. Will this only be applied to latest 4.x or also be backported to 3.x?

          Show
          Kristofer Karlsson added a comment - That looks good to me. Will this only be applied to latest 4.x or also be backported to 3.x?
          Hide
          Commit Tag Bot added a comment -

          [trunk commit] Uwe Schindler
          http://svn.apache.org/viewvc?view=revision&revision=1441726

          LUCENE-4740: Don't track clones of MMapIndexInput if unmapping is disabled. This reduces GC overhead.

          Show
          Commit Tag Bot added a comment - [trunk commit] Uwe Schindler http://svn.apache.org/viewvc?view=revision&revision=1441726 LUCENE-4740 : Don't track clones of MMapIndexInput if unmapping is disabled. This reduces GC overhead.
          Hide
          Uwe Schindler added a comment - - edited

          I committed this to trunk and 4.x branch.

          It is not yet planned to release another 3.6.x version. Users should upgrade to Lucene 4.x (4.2 will contain this patch).

          Show
          Uwe Schindler added a comment - - edited I committed this to trunk and 4.x branch. It is not yet planned to release another 3.6.x version. Users should upgrade to Lucene 4.x (4.2 will contain this patch).
          Hide
          Commit Tag Bot added a comment -

          [branch_4x commit] Uwe Schindler
          http://svn.apache.org/viewvc?view=revision&revision=1441727

          Merged revision(s) 1441726 from lucene/dev/trunk:
          LUCENE-4740: Don't track clones of MMapIndexInput if unmapping is disabled. This reduces GC overhead.

          Show
          Commit Tag Bot added a comment - [branch_4x commit] Uwe Schindler http://svn.apache.org/viewvc?view=revision&revision=1441727 Merged revision(s) 1441726 from lucene/dev/trunk: LUCENE-4740 : Don't track clones of MMapIndexInput if unmapping is disabled. This reduces GC overhead.
          Hide
          Uwe Schindler added a comment -

          I keep this issue open:

          • If there will be a 3.6.3 release, we can backport.
          • We should try/benchmark other solutions to tracking clones, as there is indeed an overhead in GC.
          Show
          Uwe Schindler added a comment - I keep this issue open: If there will be a 3.6.3 release, we can backport. We should try/benchmark other solutions to tracking clones, as there is indeed an overhead in GC.
          Hide
          Shawn Heisey added a comment -

          I've been watching the activity on this issue because I have occasional extreme GC pauses in Solr with an 8GB heap. GC tuning has reduced them somewhat so that my load balancer hasn't marked the service offline in a few days, but I think that things still aren't ideal.

          I will admit that I find most of the comments baffling. Therefore I have a simple question. If I run Solr branch_4x with this patch applied, will I benefit? I can see from the commit log that unmmapping must disabled to benefit, but I don't know if this is how Solr operates.

          Show
          Shawn Heisey added a comment - I've been watching the activity on this issue because I have occasional extreme GC pauses in Solr with an 8GB heap. GC tuning has reduced them somewhat so that my load balancer hasn't marked the service offline in a few days, but I think that things still aren't ideal. I will admit that I find most of the comments baffling. Therefore I have a simple question. If I run Solr branch_4x with this patch applied, will I benefit? I can see from the commit log that unmmapping must disabled to benefit, but I don't know if this is how Solr operates.
          Hide
          Kristofer Karlsson added a comment -

          I just had an idea for a potential optimization:
          Use a timerthread/executor/whatever to delay the unmap operation for X seconds after marking it as closed.
          This should give the clones enough time to figure out that it should fail nicely instead of crash the jvm.

          You could even have the clones make sure to check the state of the master every X/2 seconds before performing any operation:

          byte readByte() {
            if (this.closed) {
              throw ...;
            }
            long currentTime = System.currentTimeMillis();
            if (currentTime - lastCheck >= X/2) {
              lastCheck = currentTime;
              if (master.isClosed() {
                this.close();
                throw ...;
              }
             }
            return curBuf.readByte();
          }
          

          Not sure how much overhead this would mean in practice, but it would at least avoid synchronization most of the time.
          An alternative is to schedule a periodic job for each clone, checking the master state and updating the clone state. That might lead to memory leaks unless weakreferences are used, so maybe that's not actually an improvement.

          Show
          Kristofer Karlsson added a comment - I just had an idea for a potential optimization: Use a timerthread/executor/whatever to delay the unmap operation for X seconds after marking it as closed. This should give the clones enough time to figure out that it should fail nicely instead of crash the jvm. You could even have the clones make sure to check the state of the master every X/2 seconds before performing any operation: byte readByte() { if (this.closed) { throw ...; } long currentTime = System.currentTimeMillis(); if (currentTime - lastCheck >= X/2) { lastCheck = currentTime; if (master.isClosed() { this.close(); throw ...; } } return curBuf.readByte(); } Not sure how much overhead this would mean in practice, but it would at least avoid synchronization most of the time. An alternative is to schedule a periodic job for each clone, checking the master state and updating the clone state. That might lead to memory leaks unless weakreferences are used, so maybe that's not actually an improvement.
          Hide
          Uwe Schindler added a comment -

          Therefore I have a simple question. If I run Solr branch_4x with this patch applied, will I benefit? I can see from the commit log that unmmapping must disabled to benefit, but I don't know if this is how Solr operates.

          If you have a version with this patch enabled and you are using an index that changes not too often, it is better to not unmap. In that case you can pass unmap="false" as parameter to your MMapDirectoryFactory in Solr. If the index does not change too often, the overhead by a delay in unmapping the files at a lter stage does not matter. GC has less to do then.

          Show
          Uwe Schindler added a comment - Therefore I have a simple question. If I run Solr branch_4x with this patch applied, will I benefit? I can see from the commit log that unmmapping must disabled to benefit, but I don't know if this is how Solr operates. If you have a version with this patch enabled and you are using an index that changes not too often, it is better to not unmap. In that case you can pass unmap="false" as parameter to your MMapDirectoryFactory in Solr. If the index does not change too often, the overhead by a delay in unmapping the files at a lter stage does not matter. GC has less to do then.
          Hide
          Steve Rowe added a comment -

          Bulk close resolved 4.4 issues

          Show
          Steve Rowe added a comment - Bulk close resolved 4.4 issues

            People

            • Assignee:
              Uwe Schindler
              Reporter:
              Kristofer Karlsson
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development