Merge pull request #73 from maxmind/greg/no-synchronized

Remove synchronized

Author: horgh

CHANGELOG.md

@@ -10,6 +10,9 @@ CHANGELOG
   control byte in the data section rather than an
   `ArrayIndexOutOfBoundsException`. Reported by Edwin Delgado H. GitHub
   #68.
+* In order to improve performance when lookups are done from multiple
+  threads, a use of `synchronized` has been removed. GitHub #65 & #69.
+* `jackson-databind` has been upgraded to 2.11.0.
 
 1.3.1 (2020-03-03)
 ------------------

pom.xml

@@ -45,7 +45,7 @@
         <dependency>
             <groupId>com.fasterxml.jackson.core</groupId>
             <artifactId>jackson-databind</artifactId>
-            <version>2.10.3</version>
+            <version>2.11.0</version>
         </dependency>
     </dependencies>
     <build>

src/main/java/com/maxmind/db/BufferHolder.java

@@ -58,7 +58,22 @@ final class BufferHolder {
      * Returns a duplicate of the underlying ByteBuffer. The returned ByteBuffer
      * should not be shared between threads.
      */
-    synchronized ByteBuffer get() {
+    ByteBuffer get() {
+        // The Java API docs for buffer state:
+        //
+        //     Buffers are not safe for use by multiple concurrent threads. If a buffer is to be used by more than
+        //     one thread then access to the buffer should be controlled by appropriate synchronization.
+        //
+        // As such, you may think that this should be synchronized. This used to be the case, but we had several
+        // complaints about the synchronization causing contention, e.g.:
+        //
+        // * https://github.com/maxmind/MaxMind-DB-Reader-java/issues/65
+        // * https://github.com/maxmind/MaxMind-DB-Reader-java/pull/69
+        //
+        // Given that we are not modifying the original ByteBuffer in any way and all currently known and most
+        // reasonably imaginable implementations of duplicate() only do read operations on the original buffer object,
+        // the risk of not synchronizing this call seems relatively low and worth taking for the performance benefit
+        // when lookups are being done from many threads.
         return this.buffer.duplicate();
     }
 }