Use array instead of HashMap for StateNode features #4552
Conversation
Legioth
force-pushed
the
featuresArray
branch
2 times, most recently
from
c3cbe33
to
e4f50b8
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained
flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java, line 73 at r2 (raw file):
Collection<Class<? extends NodeFeature>> reportableFeatureTypes, Stream<Class<? extends NodeFeature>> allFeatures) { this(new HashSet<>(reportableFeatureTypes),
The first constructor is used in this constructor only so I think at least one of those is redundant.
I wonder why do we copy the first collection here, but don't do such thing in the first constructor.
Also I wonder why do we need this constructor at all, since the user can manage to collect the stream into the collection required himself.
flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java, line 102 at r2 (raw file):
* Maps from a node feature type to its index in the {@link #features} * array. This instance is cached per unique set of used node feature * types in {@link #nodeFeatureMappingsCache}.
Dead link to nodeFeatureMappingsCache.
flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java, line 108 at r2 (raw file):
public FeatureSet(FeatureSetKey featureSetKey) { reportedFeatures = featureSetKey.reportedFeatures; mappings = createMappings(featureSetKey.allFeatures);
If I understand in correctly, you store allFeatures twice: in the cache key and then repack those here into the map with indices.
What if we store unreportedFeatures in the key instead of allFeatures? By doing this we will store less elements in the key, the key detection should work the same and for the value we concat both sets and produce the same map with indices for the cache value.
flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java, line 195 at r2 (raw file):
Previously, vaadin-bot (Vaadin Bot) wrote…
Move this method into "FeatureSet".
Yeah.
I presume it would be better if the method is hidden in the class it's used in, as a private method rather than a static method in an enclosing class.
Or it can be simply inlined in the constructor of the class it's used.
flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java, line 310 at r2 (raw file):
private void forEachChild(Consumer<StateNode> action) { Arrays.asList(features).forEach(n -> n.forEachChild(action));
Any reason not to use plain old for loop here? It works on arrays.
(this is not the only place to fix)
There was a problem hiding this comment.
Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained
flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java, line 73 at r2 (raw file):
Previously, SomeoneToIgnore (Kirill Bulatov) wrote…
The first constructor is used in this constructor only so I think at least one of those is redundant.
I wonder why do we copy the first collection here, but don't do such thing in the first constructor.
Also I wonder why do we need this constructor at all, since the user can manage to collect the stream into the collection required himself.
Removed redundant constructor.
For this kind of private API, I prefer to accept things in the format that is most convenient for the only known caller to avoid complexity on that side.
flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java, line 102 at r2 (raw file):
Previously, SomeoneToIgnore (Kirill Bulatov) wrote…
Dead link to
nodeFeatureMappingsCache.
Done.
flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java, line 108 at r2 (raw file):
Previously, SomeoneToIgnore (Kirill Bulatov) wrote…
If I understand in correctly, you store
allFeaturestwice: in the cache key and then repack those here into the map with indices.What if we store
unreportedFeaturesin the key instead ofallFeatures? By doing this we will store less elements in the key, the key detection should work the same and for the value we concat both sets and produce the same map with indices for the cache value.
Done.
flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java, line 195 at r2 (raw file):
Previously, SomeoneToIgnore (Kirill Bulatov) wrote…
Yeah.
I presume it would be better if the method is hidden in the class it's used in, as a private method rather than a static method in an enclosing class.
Or it can be simply inlined in the constructor of the class it's used.
Done.
flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java, line 310 at r2 (raw file):
Previously, SomeoneToIgnore (Kirill Bulatov) wrote…
Any reason not to use plain old
forloop here? It works on arrays.
(this is not the only place to fix)
Done.
flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java, line 443 at r2 (raw file):
Previously, vaadin-bot (Vaadin Bot) wrote…
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained, and 1 stale
|
flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java, line 70 at r3 (raw file):
Nice catch. |
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, 1 of 1 LGTMs obtained
flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java, line 70 at r3 (raw file):
Previously, SomeoneToIgnore (Kirill Bulatov) wrote…
Nice catch.
java.lang.AssertionError: No reportable feature should also be non-reportable at com.vaadin.flow.internal.StateNode$FeatureSetKey.<init>(StateNode.java:70) at com.vaadin.flow.internal.StateNode.<init>(StateNode.java:189) at com.vaadin.flow.dom.impl.BasicElementStateProvider.createStateNode(BasicElementStateProvider.java:112) at com.vaadin.flow.dom.Element.createStateNode(Element.java:237) at com.vaadin.flow.dom.Element.<init>(Element.java:102) at com.vaadin.flow.dom.ElementFactory.createDiv(ElementFactory.java:114) at com.vaadin.flow.dom.ElementTest.addExistingClass_noop(ElementTest.java:787) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) at org.junit.runners.ParentRunner.run(ParentRunner.java:363) at org.junit.runner.JUnitCore.run(JUnitCore.java:137) at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68) at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47) at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242) at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
We're protected from the duplicates for now due to
But it looks like either some subtraction may be done around
|
flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java, line 70 at r3 (raw file): Previously, SomeoneToIgnore (Kirill Bulatov) wrote…
...or right in the place of the comment. |
There is only a limited set of different state node configurations. For each configuration, we cache a mapping from feature type to the index in an array containing the actual feature instances. This representation is much more memory efficient than the previously used generic HashMap. Reduces BasicElementView memory use from 429706 to 278950 bytes and the memory use in BeverageBuddy with an edit dialog open from 262257 to 183211 bytes.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained, and 1 stale
flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java, line 70 at r3 (raw file):
Previously, SomeoneToIgnore (Kirill Bulatov) wrote…
...or right in the place of the comment.
I blame bad variable names and inconsistent conventions between how element and text nodes are created. Fixed.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4.
Dismissed @vaadin-bot from 2 discussions.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained, and 1 stale
There is only a limited set of different state node configurations. For
each configuration, we cache a mapping from feature type to the index in
an array containing the actual feature instances. This representation is
much more memory efficient than the previously used generic HashMap.
Reduces BasicElementView memory use from 429706 to 278950 bytes and the
memory use in BeverageBuddy with an edit dialog open from 262257 to
183211 bytes.
This change is