JsonObject optimization #4994

magicprinc · 2023-12-01T23:40:47Z

use JDK Map.entry(key,value) instead of self-made Entry class

One caveat: Map.entry disallows null keys and values (Attempts to create Map.Entry using a null key or value result in NullPointerException), but it is used in JsonObject iterator only and the iterator doesn't wrap null values in new Map.Entry

(Linked)HashMap has DEFAULT_LOAD_FACTOR = 0.75f: if you create it with initialCapacity=X and put X items into it, then HashMap resizes (with new internal array allocation and re-hash). One has to create HashMap with initialCapacity=X/0.75
e.g. for 10 keys initialCapacity must be at least 14

You can see it yourself under the debugger

better inheritance: equals
if (getClass() != o.getClass()) return false rejects subclasses
if (!(o instanceof JsonObject)) return false allows subclasses and checks for null

1) use JDK Map.entry(key,value) instead of self-made Entry class One caveat: Map.entry disallows null keys and values (Attempts to create Map.Entry using a null key or value result in NullPointerException) 2) (Linked)HashMap has DEFAULT_LOAD_FACTOR = 0.75f: if you create it with initialCapacity=X and put X items into it then HashMap resizes (with new internal array allocation and re-hash). One has to create HashMap with initialCapacity=X/0.75 e.g. for 10 keys initialCapacity must be at least 14 3) better inheritance: equals `if (getClass() != o.getClass()) return false;` rejects subclasses. if (!(o instanceof JsonObject)) return false allows subclasses and checks for null

magicprinc · 2023-12-01T23:51:10Z

No problems with null
Test passes

@Test 
public void _null (){
  JsonObject jo = JsonObject.of("x", null, "", null);
  jo.stream().forEach(kv->{
    assertNull(kv.getValue());
  });
}

tsegismont

OK with a few comments

tsegismont · 2023-12-02T11:03:20Z

src/main/java/io/vertx/core/json/JsonObject.java

@@ -122,7 +122,7 @@ public static JsonObject of(String k1, Object v1) {
   * @return a JsonObject containing the specified mappings.
   */
  public static JsonObject of(String k1, Object v1, String k2, Object v2) {
-    JsonObject obj = new JsonObject(new LinkedHashMap<>(2));
+    JsonObject obj = new JsonObject(new LinkedHashMap<>(4));


Suggested change

JsonObject obj = new JsonObject(new LinkedHashMap<>(4));

JsonObject obj = new JsonObject(new LinkedHashMap<>(3));

According to https://guava.dev/releases/21.0/api/docs/src-html/com/google/common/collect/Maps.html#line.291

tsegismont · 2023-12-02T11:04:35Z

src/main/java/io/vertx/core/json/JsonObject.java

@@ -105,7 +105,7 @@ public static JsonObject of() {
   * @return a JsonObject containing the specified mapping.
   */
  public static JsonObject of(String k1, Object v1) {
-    JsonObject obj = new JsonObject(new LinkedHashMap<>(1));
+    JsonObject obj = new JsonObject(new LinkedHashMap<>(3));


Suggested change

JsonObject obj = new JsonObject(new LinkedHashMap<>(3));

JsonObject obj = new JsonObject(new LinkedHashMap<>(2));

According to https://guava.dev/releases/21.0/api/docs/src-html/com/google/common/collect/Maps.html#line.291

tsegismont · 2023-12-02T11:05:43Z

src/main/java/io/vertx/core/json/JsonObject.java

@@ -166,7 +166,7 @@ public static JsonObject of(String k1, Object v1, String k2, Object v2, String k
   */
  public static JsonObject of(String k1, Object v1, String k2, Object v2, String k3, Object v3,
                              String k4, Object v4) {
-    JsonObject obj = new JsonObject(new LinkedHashMap<>(4));
+    JsonObject obj = new JsonObject(new LinkedHashMap<>(7));


Suggested change

JsonObject obj = new JsonObject(new LinkedHashMap<>(7));

JsonObject obj = new JsonObject(new LinkedHashMap<>(6));

https://guava.dev/releases/21.0/api/docs/src-html/com/google/common/collect/Maps.html#line.291

tsegismont · 2023-12-02T11:06:08Z

src/main/java/io/vertx/core/json/JsonObject.java

@@ -193,7 +193,7 @@ public static JsonObject of(String k1, Object v1, String k2, Object v2, String k
   */
  public static JsonObject of(String k1, Object v1, String k2, Object v2, String k3, Object v3,
                              String k4, Object v4, String k5, Object v5) {
-    JsonObject obj = new JsonObject(new LinkedHashMap<>(5));
+    JsonObject obj = new JsonObject(new LinkedHashMap<>(8));


Suggested change

JsonObject obj = new JsonObject(new LinkedHashMap<>(8));

JsonObject obj = new JsonObject(new LinkedHashMap<>(7));

https://guava.dev/releases/21.0/api/docs/src-html/com/google/common/collect/Maps.html#line.291

tsegismont · 2023-12-02T11:07:02Z

src/main/java/io/vertx/core/json/JsonObject.java

@@ -294,7 +294,7 @@ public static JsonObject of(String k1, Object v1, String k2, Object v2, String k
  public static JsonObject of(String k1, Object v1, String k2, Object v2, String k3, Object v3,
                              String k4, Object v4, String k5, Object v5, String k6, Object v6,
                              String k7, Object v7, String k8, Object v8) {
-    JsonObject obj = new JsonObject(new LinkedHashMap<>(8));
+    JsonObject obj = new JsonObject(new LinkedHashMap<>(12));


Suggested change

JsonObject obj = new JsonObject(new LinkedHashMap<>(12));

JsonObject obj = new JsonObject(new LinkedHashMap<>(11));

https://guava.dev/releases/21.0/api/docs/src-html/com/google/common/collect/Maps.html#line.291

tsegismont · 2023-12-02T11:07:33Z

src/main/java/io/vertx/core/json/JsonObject.java

@@ -334,7 +334,7 @@ public static JsonObject of(String k1, Object v1, String k2, Object v2, String k
  public static JsonObject of(String k1, Object v1, String k2, Object v2, String k3, Object v3,
                              String k4, Object v4, String k5, Object v5, String k6, Object v6,
                              String k7, Object v7, String k8, Object v8, String k9, Object v9) {
-    JsonObject obj = new JsonObject(new LinkedHashMap<>(9));
+    JsonObject obj = new JsonObject(new LinkedHashMap<>(14));


Suggested change

JsonObject obj = new JsonObject(new LinkedHashMap<>(14));

JsonObject obj = new JsonObject(new LinkedHashMap<>(13));

https://guava.dev/releases/21.0/api/docs/src-html/com/google/common/collect/Maps.html#line.291

tsegismont · 2023-12-02T11:07:46Z

src/main/java/io/vertx/core/json/JsonObject.java

@@ -378,7 +378,7 @@ public static JsonObject of(String k1, Object v1, String k2, Object v2, String k
                              String k4, Object v4, String k5, Object v5, String k6, Object v6,
                              String k7, Object v7, String k8, Object v8, String k9, Object v9,
                              String k10, Object v10) {
-    JsonObject obj = new JsonObject(new LinkedHashMap<>(10));
+    JsonObject obj = new JsonObject(new LinkedHashMap<>(16));


Suggested change

JsonObject obj = new JsonObject(new LinkedHashMap<>(16));

JsonObject obj = new JsonObject(new LinkedHashMap<>(14));

https://guava.dev/releases/21.0/api/docs/src-html/com/google/common/collect/Maps.html#line.291

tsegismont · 2023-12-02T11:10:26Z

src/main/java/io/vertx/core/json/JsonObject.java

@@ -1160,14 +1160,11 @@ public String toString() {

  @Override
  public boolean equals(Object o) {
-    // null check


We don't need to remove this, unless it is proved it slows down things

tsegismont · 2023-12-02T11:11:36Z

src/main/java/io/vertx/core/json/JsonObject.java

    // self check
    if (this == o)
      return true;
-    // type check and cast
-    if (getClass() != o.getClass())
+    // type check, cast and null check


No need to change this, we don't plan to support subclasses

src/main/java/io/vertx/core/json/JsonObject.java

magicprinc · 2023-12-02T12:56:33Z

@tsegismont I have indeed made initial capacity bigger than required, but only to prevent resize for the next 1-3 user’s puts. Do you have any statistics how JsonObject is used? I am not sure what is better 🤷‍♀️

Following question: does JsonObject really needs LinkedHashMap? LinkedHashMap uses more memory than HashMap. The only place where you see the difference is interator().
And by the way: JsonObjects brokes this contract already in copy()

About equals: why not make it final, then?
I.e., make final, remove null check, use instanceof ?

1) skip implicit null check 2) MAP_CREATOR

1) use MAP_CREATOR 2) LinkedHashMap → HashMap (can be changed in MAP_CREATOR) 3) better initial capacity

magicprinc · 2023-12-02T14:04:17Z

@tsegismont Please review again: I have found a very interesting solution.

better spliterator() and stream() implementations Use Spliterator flags and map.size() to make them more performant

vietj · 2023-12-04T08:05:09Z

src/main/java/io/vertx/core/json/JsonObject.java

@@ -1158,16 +1182,21 @@ public String toString() {
    return encode();
  }

+  /**
+   How to Write an Equality Method in Java


can you remove this reference to this article ? it's not needed

vietj · 2023-12-04T08:07:16Z

src/main/java/io/vertx/core/json/JsonObject.java

+   How to Write an Equality Method in Java
+   https://www.artima.com/articles/how-to-write-an-equality-method-in-java
+   */
+  protected boolean canEqual(Object other) {


can you actually remove this ? we don't actually add such contract for json object.

json object is not meant to be subclasses (although sometimes it is) and we don't encourage it.

vietj · 2023-12-04T08:09:22Z

src/main/java/io/vertx/core/json/impl/JsonUtil.java

+  // todo replace with HashMap? → Less memory usage, but random keys order, but .stream().sorted()?
+  // immutable Map.of javadoc: The iteration order of mappings is unspecified and is subject to change
+  @SuppressWarnings("rawtypes")
+  public static final IntFunction<Map> MAP_CREATOR = size -> new LinkedHashMap(size < 3


please inline that directly in map(int), we don't need to have one static method that delegates to one static field

magicprinc · 2023-12-04T09:37:05Z

@tsegismont @vietj I will redo this PR as a single commit with all fixes.

tsegismont · 2023-12-04T09:37:38Z

@magicprinc please keep only the following changes in your PR:

capacity of the map in JsonObject.of methods
remove redundant class

We don't need a method for dynamic computation of map capacity. For JsonObject.of methods, we can use a static value which is enough to store the parameters (these methods are generally used to create Json objects which won't be modified).

magicprinc · 2023-12-04T11:01:53Z

I did my best #5001

vietj · 2023-12-04T11:04:15Z

thanks, you didn't need to close the PR for a new one, you could have force pushed your branch instead with the rebased version, that would have updated the PR in place

that is preferable because with a new PR with lose the context of the review in progress.

magicprinc · 2023-12-04T11:19:44Z

@vietj I didn't know such thing exists. Thank You!

magicprinc added 2 commits December 2, 2023 02:31

fix imports

72d7a7b

tsegismont reviewed Dec 2, 2023

View reviewed changes

magicprinc added 4 commits December 2, 2023 16:31

code review

d0cf9bc

JsonUtil

3b93c5b

1) skip implicit null check 2) MAP_CREATOR

JsonObject: +map

b9d8eeb

1) use MAP_CREATOR 2) LinkedHashMap → HashMap (can be changed in MAP_CREATOR) 3) better initial capacity

+ test & fix tests

d33b4e9

magicprinc added 3 commits December 2, 2023 17:19

JsonObject: stream, spliterator

d5704e7

better spliterator() and stream() implementations Use Spliterator flags and map.size() to make them more performant

more about order; better naming; Helpful NPE

9325652

fix equals

b425eea

vietj reviewed Dec 4, 2023

View reviewed changes

Merge branch 'eclipse-vertx:master' into feature/JsonObject

4d0010a

magicprinc mentioned this pull request Dec 4, 2023

JsonObject optimizations #5001

Open

magicprinc closed this Dec 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JsonObject optimization #4994

JsonObject optimization #4994

magicprinc commented Dec 1, 2023 •

edited

magicprinc commented Dec 1, 2023

tsegismont left a comment

tsegismont Dec 2, 2023

tsegismont Dec 2, 2023

tsegismont Dec 2, 2023

tsegismont Dec 2, 2023

tsegismont Dec 2, 2023

tsegismont Dec 2, 2023

tsegismont Dec 2, 2023

tsegismont Dec 2, 2023

tsegismont Dec 2, 2023

magicprinc commented Dec 2, 2023 •

edited

magicprinc commented Dec 2, 2023

vietj Dec 4, 2023

vietj Dec 4, 2023

vietj Dec 4, 2023

magicprinc commented Dec 4, 2023

tsegismont commented Dec 4, 2023

magicprinc commented Dec 4, 2023

vietj commented Dec 4, 2023 •

edited

magicprinc commented Dec 4, 2023

	JsonObject obj = new JsonObject(new LinkedHashMap<>(4));
	JsonObject obj = new JsonObject(new LinkedHashMap<>(3));

JsonObject optimization #4994

JsonObject optimization #4994

Conversation

magicprinc commented Dec 1, 2023 • edited

magicprinc commented Dec 1, 2023

tsegismont left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magicprinc commented Dec 2, 2023 • edited

magicprinc commented Dec 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magicprinc commented Dec 4, 2023

tsegismont commented Dec 4, 2023

magicprinc commented Dec 4, 2023

vietj commented Dec 4, 2023 • edited

magicprinc commented Dec 4, 2023

magicprinc commented Dec 1, 2023 •

edited

magicprinc commented Dec 2, 2023 •

edited

vietj commented Dec 4, 2023 •

edited