Use a simpler representation for NodeMap with only one value

[Legioth]

A HashMap with only one item uses lots of memory since the hash table
has room for 16 items, and each actually used item in the array is a
linked list node. Collections.singletonMap gets rid of this overhead,
but still uses memory for caching keySet, entrySet and values collection
view instances.

Reduces BasicElementView memory use from 429706 to 378338 bytes and the
memory use in BeverageBuddy with an edit dialog open from 262257 to
230435 bytes.

---

This change is 

[vaadin-bot]

Remove this method to simply inherit it.

[SomeoneToIgnore]

Reviewed 1 of 1 files at r1.
Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained

---

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeMap.java, line 67 at r1 (raw file):

        private final String key;

        private Serializable value;

Looks like we don't change the value after setting it, so we can have it as final also.

---

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeMap.java, line 101 at r1 (raw file):

        @Override
        public Stream<Serializable> streamValues() {
            return Stream.of(value);

This is able to throw NPE when the value is null.
Should we assert it in the constructor? Alternatively, we can return an empty stream in this case.

---

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeMap.java, line 112 at r1 (raw file):

Previously, vaadin-bot (Vaadin Bot) wrote…

Prefix this call to "put" with "super.".

Looks legit, also there's another similar one. Also I wonder why do we extend the HashMap at all instead of hiding it as a field – we might expose the methods we don't want to expose at least.

---

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeMap.java, line 117 at r1 (raw file):

Previously, vaadin-bot (Vaadin Bot) wrote…

Remove this method to simply inherit it.

This is related to the previous Sonar message – either we store hashmap as a field and have these methods or we remove those two.

---

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeMap.java, line 189 at r1 (raw file):

            values = new SingleValue(key, value);
        } else {
            if (!(values instanceof HashMapValues)) {

Optional, since require noticeable changes to the PR.

If I understand what's happening here correctly, we start with a single-sized collection and then replace it with multiple sized and nothing else is done.

If that's true, It's possible to simplify the code and remove casts and instanceof checks completely.

What I would do is to remove the SingleValue and HashMapValues completely and, most probably, Values interface also.
Then I'd create a simple class that has a values field and the methods required (put, get etc.).
Then, on the element added, I'd check the size of the current map, if it's absent ot zero, initialize it with a single value (or singleton map), otherwise replace it with a regular map.

This will help to encapsulate the resize logic into a single class that can be unit tested, reduce the code in this class and remove all the current sonar warnings, instanceof checks, casts and HashMap extensions.

[vaadin-bot]

Remove this method to simply inherit it.

[vaadin-bot]

SonarQube analysis reported 2 issues

• 2 minor

Watch the comments in this conversation to review them.

[SomeoneToIgnore]

Reviewed 2 of 2 files at r2.
Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained

[Legioth]

Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained

---

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeMap.java, line 67 at r1 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…

Looks like we don't change the value after setting it, so we can have it as final also.

It's the opposite: It's a bug that put("foo", "bar); put("foo", "baz") will make it allocate a full-size HashMap instead of reassigning this field. Fixing the bug and adding a test for the case.

---

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeMap.java, line 101 at r1 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…

This is able to throw NPE when the value is null.
Should we assert it in the constructor? Alternatively, we can return an empty stream in this case.

This compiles into an invocation of Stream.of(Object) which gives a stream containing a null value. Not to be confused with Stream.of(Object ...) which would indeed throw.

Added a test for the case just to be on the safe side.

---

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeMap.java, line 112 at r1 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…

Looks legit, also there's another similar one. Also I wonder why do we extend the HashMap at all instead of hiding it as a field – we might expose the methods we don't want to expose at least.

Done.

The reason for extending instead of wrapping HashMap is to keep memory overhead at a minimum, at the expense of simplicity.

---

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeMap.java, line 117 at r1 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…

This is related to the previous Sonar message – either we store hashmap as a field and have these methods or we remove those two.

This is a false positive from sonar because it doesn't consider type erasure. Without this method, the compiler complains that Values.get isn't implemented.

---

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeMap.java, line 189 at r1 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…

Optional, since require noticeable changes to the PR.

If I understand what's happening here correctly, we start with a single-sized collection and then replace it with multiple sized and nothing else is done.

If that's true, It's possible to simplify the code and remove casts and instanceof checks completely.

What I would do is to remove the SingleValue and HashMapValues completely and, most probably, Values interface also.
Then I'd create a simple class that has a values field and the methods required (put, get etc.).
Then, on the element added, I'd check the size of the current map, if it's absent ot zero, initialize it with a single value (or singleton map), otherwise replace it with a regular map.

This will help to encapsulate the resize logic into a single class that can be unit tested, reduce the code in this class and remove all the current sonar warnings, instanceof checks, casts and HashMap extensions.

This is an extreme memory optimization that unfortunately causes some additional complexity in the code. The additional indirection that you suggest would cost us 16 bytes for the object header and another 8 bytes for an inner values field. (and half of that in 32-bit mode).

[Legioth]

Dismissed @vaadin-bot from a discussion.
Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained

[SomeoneToIgnore]

Dismissed @vaadin-bot from 2 discussions.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained, and 1 stale

---

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeMap.java, line 189 at r1 (raw file):

Previously, Legioth (Leif Åstrand) wrote…

This is an extreme memory optimization that unfortunately causes some additional complexity in the code. The additional indirection that you suggest would cost us 16 bytes for the object header and another 8 bytes for an inner values field. (and half of that in 32-bit mode).

Oh, ok, got no idea that this is so extreme.