Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-7355

Java 8: ConcurrentLinkedHashMap -> Caffeine

    Details

    • Type: Task
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.0
    • Component/s: None
    • Labels:
      None

      Description

      When Solr transitions to requiring Java 8, please upgrade to Caffeine. The performance should be relatively the same. The per-instance memory usage should be smaller and Solr may decide to opt-in to use some of the additional features. The only drawback is that the jar size is larger due to code generation, though that may be trimmed over time and usually is not a concern for server-side applications.

      ConcurrentLinkedHashMap changes will continue to be minimal, even more so now, and driven by requests from Java 6 users unable to upgrade. Caffeine is ideally the upgrade path for Guava cache users too, which due to Android cannot be significantly modified.

      Caffeine: https://github.com/ben-manes/caffeine
      Benchmarks: https://github.com/ben-manes/caffeine/wiki/Benchmarks
      ConcurrentLinkedHashMap: https://code.google.com/p/concurrentlinkedhashmap

      1. SOLR-7355.patch
        33 kB
        Shawn Heisey
      2. SOLR-7355.patch
        8 kB
        Shawn Heisey
      3. SOLR-7355.patch
        20 kB
        Shawn Heisey
      4. SOLR-7355.patch
        32 kB
        Ben Manes

        Issue Links

          Activity

          Hide
          elyograg Shawn Heisey added a comment -

          The SVN trunk (6.0.0-SNAPSHOT) already requires Java 8. There's a chance that branch_5x will be upgraded to require Java 8 before it is retired to maintenance mode, in the same way that we bumped our requirements to Java 7 with the release of 4.8.0.

          If you check out the trunk code, you can patch it and provide repeatable benchmarks to prove to us that your library will be stable and provide a performance advantage.

          http://wiki.apache.org/solr/HowToContribute

          Show
          elyograg Shawn Heisey added a comment - The SVN trunk (6.0.0-SNAPSHOT) already requires Java 8. There's a chance that branch_5x will be upgraded to require Java 8 before it is retired to maintenance mode, in the same way that we bumped our requirements to Java 7 with the release of 4.8.0. If you check out the trunk code, you can patch it and provide repeatable benchmarks to prove to us that your library will be stable and provide a performance advantage. http://wiki.apache.org/solr/HowToContribute
          Hide
          ben.manes Ben Manes added a comment -

          This ticket is not an attempt to advocate a competing project, but rather is a notification that ConcurrentLinkedHashMap has reached its end-of-life and will only receive maintenance updates. Similarly Guava's cache is in maintenance mode, performance improvements are not accepted due to a lack of owner, and Guava stay JDK6 due to Android. As the author or co-author of all three caching libraries, this is the upgrade path that I recommend. Solr can instead choose to stay as is or migrate to an alternative library.

          Does Solr have performance tests? The before mentioned benchmarks show that Caffeine has equivalent performance to CLHM v1.4.2. However, Solr uses the older v1.2 dependency which typically performs reasonably well under application load but much worse in a micro-benchmark. That's due to the original focus on avoiding lock contention (Java 5 synchronizers were slow) and later resolving GC pressure under synthetic stress tests. The evolutionary improvements to Caffeine's algorithms mostly reduce memory usage and has only a subtle impact on the performance profile.

          Caffeine executes over 1.5 million unit tests and the changes pass Solr's test suite. Any latent bugs will be addressed upon discovery.

          Show
          ben.manes Ben Manes added a comment - This ticket is not an attempt to advocate a competing project, but rather is a notification that ConcurrentLinkedHashMap has reached its end-of-life and will only receive maintenance updates. Similarly Guava's cache is in maintenance mode, performance improvements are not accepted due to a lack of owner, and Guava stay JDK6 due to Android. As the author or co-author of all three caching libraries, this is the upgrade path that I recommend. Solr can instead choose to stay as is or migrate to an alternative library. Does Solr have performance tests? The before mentioned benchmarks show that Caffeine has equivalent performance to CLHM v1.4.2. However, Solr uses the older v1.2 dependency which typically performs reasonably well under application load but much worse in a micro-benchmark. That's due to the original focus on avoiding lock contention (Java 5 synchronizers were slow) and later resolving GC pressure under synthetic stress tests. The evolutionary improvements to Caffeine's algorithms mostly reduce memory usage and has only a subtle impact on the performance profile. Caffeine executes over 1.5 million unit tests and the changes pass Solr's test suite. Any latent bugs will be addressed upon discovery.
          Hide
          elyograg Shawn Heisey added a comment -

          I personally am ignorant about both Google's code and your code. Given that, I have two primary concerns:

          • Making sure the license is compatible and will remain compatible for the foreseeable future.
          • Ensuring that we don't take a step backwards in performance, and ideally, that performance is improved.

          If these concerns are not a problem, then I have no opposition to your patch going in, but others may have technical questions or concerns. If any of those come up, they will need to be addressed.

          Show
          elyograg Shawn Heisey added a comment - I personally am ignorant about both Google's code and your code. Given that, I have two primary concerns: Making sure the license is compatible and will remain compatible for the foreseeable future. Ensuring that we don't take a step backwards in performance, and ideally, that performance is improved. If these concerns are not a problem, then I have no opposition to your patch going in, but others may have technical questions or concerns. If any of those come up, they will need to be addressed.
          Hide
          elyograg Shawn Heisey added a comment -

          Is the use of Google's hashmap implementation limited to Solr, or does it go all the way down to Lucene? There are a number of Lucene benchmarks that are automatically run by our CI servers with each new code update, but I do not know of any specifically for Solr.

          Show
          elyograg Shawn Heisey added a comment - Is the use of Google's hashmap implementation limited to Solr, or does it go all the way down to Lucene? There are a number of Lucene benchmarks that are automatically run by our CI servers with each new code update, but I do not know of any specifically for Solr.
          Hide
          ben.manes Ben Manes added a comment -
          • License remains Apache 2.
          • Performance should be the same or higher.
          • A grep for Google's shows it being used primarily for memoization.
          • Solr appears to use many different caching implementations so its hard, as an outsider, to discern which are performance critical.
          Show
          ben.manes Ben Manes added a comment - License remains Apache 2. Performance should be the same or higher. A grep for Google's shows it being used primarily for memoization. Solr appears to use many different caching implementations so its hard, as an outsider, to discern which are performance critical.
          Hide
          elyograg Shawn Heisey added a comment -

          Running 'ant precommit' turned up a missing eol-style on the new license file. Attached patch fixes that. Running precommit again.

          Show
          elyograg Shawn Heisey added a comment - Running 'ant precommit' turned up a missing eol-style on the new license file. Attached patch fixes that. Running precommit again.
          Hide
          ben.manes Ben Manes added a comment -

          Thanks! I thought I fixed that in the last update (accidentally two license files), but looking at it I guess I uploaded the stale patch.

          Show
          ben.manes Ben Manes added a comment - Thanks! I thought I fixed that in the last update (accidentally two license files), but looking at it I guess I uploaded the stale patch.
          Hide
          elyograg Shawn Heisey added a comment -

          My patch was actually against 5x, which I realized was wrong because it only requires Java 7. I'm still working on precommit, this time against trunk. There are a few more problems.

          The precommit target requires a "python3.2" binary. You can use a later python release, as long as you symlink or copy the executable so that one exists for python3.2 as well.

          Show
          elyograg Shawn Heisey added a comment - My patch was actually against 5x, which I realized was wrong because it only requires Java 7. I'm still working on precommit, this time against trunk. There are a few more problems. The precommit target requires a "python3.2" binary. You can use a later python release, as long as you symlink or copy the executable so that one exists for python3.2 as well.
          Hide
          ben.manes Ben Manes added a comment -

          I saw that too and assumed python3 had to be available, so I brew installed it. The precommit was successful for me, but for the patch that I accidentally didn't upload.

          Show
          ben.manes Ben Manes added a comment - I saw that too and assumed python3 had to be available, so I brew installed it. The precommit was successful for me, but for the patch that I accidentally didn't upload.
          Hide
          elyograg Shawn Heisey added a comment -

          One thing missing is solr/licenses/caffeine-NOTICE.txt – what should this file contain?

          Show
          elyograg Shawn Heisey added a comment - One thing missing is solr/licenses/caffeine-NOTICE.txt – what should this file contain?
          Hide
          ben.manes Ben Manes added a comment -

          Odd that the patch doesn't show that this was moved,
          A + solr/licenses/caffeine-NOTICE.txt
          > moved from solr/licenses/concurrentlinkedhashmap-lru-NOTICE.txt

          The CLHM was empty, which is acceptable for Caffeine as well.

          Show
          ben.manes Ben Manes added a comment - Odd that the patch doesn't show that this was moved, A + solr/licenses/caffeine-NOTICE.txt > moved from solr/licenses/concurrentlinkedhashmap-lru-NOTICE.txt The CLHM was empty, which is acceptable for Caffeine as well.
          Hide
          elyograg Shawn Heisey added a comment -

          It seems that CLHM is only used by BlockCache, which is a cache type that I didn't know existed. The only place this cache seems to be used is HdfsDirectoryFactory and associated tests, so I think that someone from Cloudera will probably need to take a look for a technical discussion. This definitely would not affect the majority of Solr users.

          Show
          elyograg Shawn Heisey added a comment - It seems that CLHM is only used by BlockCache, which is a cache type that I didn't know existed. The only place this cache seems to be used is HdfsDirectoryFactory and associated tests, so I think that someone from Cloudera will probably need to take a look for a technical discussion. This definitely would not affect the majority of Solr users.
          Hide
          elyograg Shawn Heisey added a comment -

          My svn version is 1.8.8 (newest seems to be 1.8.13). I see that "svn stat" classifies the new license files for Caffeine as moves, but "svn diff" is not including those moves in the patch.

          Mark Miller, can you comment on ConcurrentLinkedHashMap usage in Solr's HDFS support and whether it would be worthwhile to switch to this library from Ben Manes? My local trunk checkout is ready for a commit, if you think it's a good idea. Both precommit and solr tests are passing.

          Show
          elyograg Shawn Heisey added a comment - My svn version is 1.8.8 (newest seems to be 1.8.13). I see that "svn stat" classifies the new license files for Caffeine as moves, but "svn diff" is not including those moves in the patch. Mark Miller , can you comment on ConcurrentLinkedHashMap usage in Solr's HDFS support and whether it would be worthwhile to switch to this library from Ben Manes ? My local trunk checkout is ready for a commit, if you think it's a good idea. Both precommit and solr tests are passing.
          Hide
          elyograg Shawn Heisey added a comment -

          New patch, which svn is not creating correctly, so it doesn't reflect changes in the solr/licenses directory. If local changes are committed, I think it will be fine.

          Show
          elyograg Shawn Heisey added a comment - New patch, which svn is not creating correctly, so it doesn't reflect changes in the solr/licenses directory. If local changes are committed, I think it will be fine.
          Hide
          markrmiller@gmail.com Mark Miller added a comment -

          Shawn Heisey, seems like it's the right upgrade path.

          Show
          markrmiller@gmail.com Mark Miller added a comment - Shawn Heisey , seems like it's the right upgrade path.
          Hide
          elyograg Shawn Heisey added a comment -

          When I tried to commit, I got an error message about "Inconsistent line ending style". The same error has been happening on "svn diff".

          I've redone the change as straight adds and deletes, no moves. That got rid of the message on "svn diff". Checking precommit and tests again just to be sure I haven't made a mistake.

          Show
          elyograg Shawn Heisey added a comment - When I tried to commit, I got an error message about "Inconsistent line ending style". The same error has been happening on "svn diff". I've redone the change as straight adds and deletes, no moves. That got rid of the message on "svn diff". Checking precommit and tests again just to be sure I haven't made a mistake.
          Hide
          elyograg Shawn Heisey added a comment -

          New patch which should be 100% complete.

          Show
          elyograg Shawn Heisey added a comment - New patch which should be 100% complete.
          Hide
          erickerickson Erick Erickson added a comment -

          One of the line-ending style error message I get when checking in patches produced by others can often by fixed by setting the line-ending to "native" with this command:

          svn propset svn:eol-style native <file>

          FWIW

          Show
          erickerickson Erick Erickson added a comment - One of the line-ending style error message I get when checking in patches produced by others can often by fixed by setting the line-ending to "native" with this command: svn propset svn:eol-style native <file> FWIW
          Hide
          thetaphi Uwe Schindler added a comment -

          My svn version is 1.8.8 (newest seems to be 1.8.13). I see that "svn stat" classifies the new license files for Caffeine as moves, but "svn diff" is not including those moves in the patch.

          You have to put --show-copies-as-adds into your svn diff command. In that case moves and copies are showing up in a diff as you would expect. Otherwise those patchs can only be applied with svn patch, because the moves/deletes/renames are only listed in the header of the hunks (the header of the hunk only shows old path and new path of file, but no diffs of files itsself, because a move does not change contents) for standard svn diff.

          Show
          thetaphi Uwe Schindler added a comment - My svn version is 1.8.8 (newest seems to be 1.8.13). I see that "svn stat" classifies the new license files for Caffeine as moves, but "svn diff" is not including those moves in the patch. You have to put --show-copies-as-adds into your svn diff command. In that case moves and copies are showing up in a diff as you would expect. Otherwise those patchs can only be applied with svn patch , because the moves/deletes/renames are only listed in the header of the hunks (the header of the hunk only shows old path and new path of file, but no diffs of files itsself, because a move does not change contents) for standard svn diff.
          Hide
          elyograg Shawn Heisey added a comment -

          Erick: I did set the eol-style on the new files, required to pass precommit. It's thoroughly mystifying.

          Uwe: I tried building the patch with that option and it didn't change. Could be a bug with that specific svn version (from an ubuntu package).

          Show
          elyograg Shawn Heisey added a comment - Erick: I did set the eol-style on the new files, required to pass precommit. It's thoroughly mystifying. Uwe: I tried building the patch with that option and it didn't change. Could be a bug with that specific svn version (from an ubuntu package).
          Hide
          thetaphi Uwe Schindler added a comment -

          Erick: I did set the eol-style on the new files, required to pass precommit. It's thoroughly mystifying.

          Those eol-style setting are not used if you apply the patch with standard patch command. "svn patch" applies those settings. So you have 2 options:

          • apply patch with "svn patch" command - this will handle all renames/moves/deletes and also properties
          • use default patch command of OS, but this will not handle moves/deletes and not apply properties. You have to do all of this manually after patching (svn add new files, svn remove old files, add properties).
          Show
          thetaphi Uwe Schindler added a comment - Erick: I did set the eol-style on the new files, required to pass precommit. It's thoroughly mystifying. Those eol-style setting are not used if you apply the patch with standard patch command. "svn patch" applies those settings. So you have 2 options: apply patch with "svn patch" command - this will handle all renames/moves/deletes and also properties use default patch command of OS, but this will not handle moves/deletes and not apply properties. You have to do all of this manually after patching (svn add new files, svn remove old files, add properties).
          Show
          thetaphi Uwe Schindler added a comment - http://svnbook.red-bean.com/en/1.7/svn.ref.svn.html#svn.ref.svn.sw.show_copies_as_adds http://stackoverflow.com/questions/317649/how-to-make-svn-diff-produce-file-that-patch-would-apply-when-svn-cp-or-svn-mv
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1672133 from Shawn Heisey in branch 'dev/trunk'
          [ https://svn.apache.org/r1672133 ]

          SOLR-7355: Switch from ConcurrentLinkedHashMap to Caffeine. Trunk-only change, as it requires java 8.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1672133 from Shawn Heisey in branch 'dev/trunk' [ https://svn.apache.org/r1672133 ] SOLR-7355 : Switch from ConcurrentLinkedHashMap to Caffeine. Trunk-only change, as it requires java 8.
          Hide
          elyograg Shawn Heisey added a comment -

          Finally committed. First test run failed with a single-test failure that I see in recent email from Jenkins. Second run passed.

          Show
          elyograg Shawn Heisey added a comment - Finally committed. First test run failed with a single-test failure that I see in recent email from Jenkins. Second run passed.
          Hide
          thetaphi Uwe Schindler added a comment -

          I somehow miss the version number in ivy-versions.properties. Maybe you only committed Solr subdir?

          Show
          thetaphi Uwe Schindler added a comment - I somehow miss the version number in ivy-versions.properties. Maybe you only committed Solr subdir?
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1672151 from Shawn Heisey in branch 'dev/trunk'
          [ https://svn.apache.org/r1672151 ]

          SOLR-7355: Accidentally committed from solr directory before.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1672151 from Shawn Heisey in branch 'dev/trunk' [ https://svn.apache.org/r1672151 ] SOLR-7355 : Accidentally committed from solr directory before.
          Hide
          erickerickson Erick Erickson added a comment -

          Uwe:

          I usually see eol-style errors during precommit when I'm applying
          other people's patches. Thanks for the tip though, I'll use it on my
          patches...

          Show
          erickerickson Erick Erickson added a comment - Uwe: I usually see eol-style errors during precommit when I'm applying other people's patches. Thanks for the tip though, I'll use it on my patches...

            People

            • Assignee:
              elyograg Shawn Heisey
              Reporter:
              ben.manes Ben Manes
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development