reiern70 commented on code in PR #1595:
URL: https://github.com/apache/wicket/pull/1595#discussion_r3991281296
##########
wicket-core/src/main/java/org/apache/wicket/ajax/AbstractDefaultAjaxBehavior.java:
##########
@@ -89,12 +89,6 @@ protected void onBind()
final Component component = getComponent();
component.setOutputMarkupId(true);
-
Review Comment:
Why is this no longer needed?
##########
wicket-core/src/main/java/org/apache/wicket/core/util/resource/locator/ExtensionResourceNameIterator.java:
##########
@@ -42,13 +42,14 @@ public class ExtensionResourceNameIterator implements
Iterator<String>
public ExtensionResourceNameIterator(final Iterable<String> extensions)
{
// Fail safe: hasNext() needs to return at least once with true
- if (extensions == null || !extensions.iterator().hasNext())
+ Iterator<String> extensionIterator = extensions == null ? null
: extensions.iterator();
+ if (extensionIterator == null || !extensionIterator.hasNext())
{
this.iterator = NULL_ITERABLE.iterator();
Review Comment:
Is this thread safe? If not... is this not important?
##########
wicket-core-tests/src/test/java/org/apache/wicket/behavior/BehaviorTest.java:
##########
@@ -89,6 +89,21 @@ public void consecutiveTemporaryBehaviorsAreRemoved() {
assertFalse(container.getBehaviors().contains(temp2));
}
+ @Test
+ void nullBehaviorIsRejectedBeforeAnythingIsStored() {
Review Comment:
JavaDoc?
##########
wicket-core/src/main/java/org/apache/wicket/ComponentState.java:
##########
@@ -0,0 +1,871 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import org.apache.wicket.behavior.Behavior;
+import org.apache.wicket.behavior.InvalidBehaviorIdException;
+import org.apache.wicket.model.IModel;
+
+/**
+ * This class keeps track of the flexible state of a component: model,
behaviors and meta data.
+ * These types of state vary per component. Not every component contains these
elements or their
+ * numbers differ. The state is stored in the {@code data} field in {@link
Component}. To keep the
+ * size of this state as small as possible, the following cases are identified:
+ * <ul>
+ * <li>No state at all: {@code data} is {@code null}
+ * <li>Only a model: {@code data} contains the model
Review Comment:
Not belonging to this PR... but many times we have components with several
models and then we need to remmeber to manually detach them... maybe wicket
should support registering other models and detaching them?
##########
wicket-benchmarks/src/main/java/org/apache/wicket/benchmarks/ComponentStateBenchmark.java:
##########
@@ -0,0 +1,466 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.benchmarks;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.wicket.AttributeModifier;
+import org.apache.wicket.Component;
+import org.apache.wicket.MetaDataKey;
+import org.apache.wicket.ajax.AjaxEventBehavior;
+import org.apache.wicket.ajax.AjaxRequestTarget;
+import org.apache.wicket.behavior.Behavior;
+import org.apache.wicket.markup.html.WebMarkupContainer;
+import org.apache.wicket.model.Model;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Level;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.OutputTimeUnit;
+import org.openjdk.jmh.annotations.Param;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.Threads;
+import org.openjdk.jmh.annotations.TearDown;
+import org.openjdk.jmh.annotations.Warmup;
+import org.openjdk.jmh.infra.Blackhole;
+
+/**
+ * Benchmarks the per-request accessors on {@link Component}'s flexible state
(model, behaviors and
+ * meta data) plus the mutate-and-detach cycle.
+ * <p>
+ * Deliberately written against public Wicket API only, so that the exact same
source can be run
+ * against different implementations of the state storage and compared.
+ * <p>
+ * Three things are measured separately, because they answer different
questions:
+ * <ul>
+ * <li>{@code read*} - the cost of reading state, per state shape. Reads do
not mutate, so a
+ * trial-scoped component is correct here and no per-invocation harness
overhead is paid.
+ * <li>{@code read*MixedShapes} - the same reads, but over a component array
holding every shape at
+ * once. This is the interesting one: with a single shape the call sites
inside the state lookup are
+ * monomorphic and inline, which flatters any implementation that dispatches
on the shape. Real
+ * pages interleave shapes. A large gap between the per-shape and mixed
numbers is the signature of
+ * dispatch that stopped inlining.
+ * <li>{@code buildAndDetach} - construct a component, populate its state and
detach it, as one
+ * operation. Detaching mutates state (temporary behaviors are removed, arrays
are compacted), so it
+ * cannot be measured repeatedly against the same instance; folding
construction into the operation
+ * keeps every invocation doing the real work without resorting to {@code
Level.Invocation}.
+ * </ul>
+ * Single threaded on purpose: component state is per component and never
contended, so extra
+ * threads measure nothing new while making the Wicket thread-local setup
harder to get right.
+ * <p>
+ * Always run with {@code -prof gc}: {@code gc.alloc.rate.norm} (bytes per
operation) is the number
+ * that matters for a framework that has to keep many pages in memory, and it
is far more stable
+ * than throughput.
+ */
+@BenchmarkMode(Mode.AverageTime)
+@OutputTimeUnit(TimeUnit.NANOSECONDS)
+@Fork(3)
+@Threads(1)
+@Warmup(iterations = 3, time = 2, timeUnit = TimeUnit.SECONDS)
+@Measurement(iterations = 5, time = 2, timeUnit = TimeUnit.SECONDS)
+public class ComponentStateBenchmark
+{
+ static final MetaDataKey<String> KEY = new MetaDataKey<>()
+ {
+ private static final long serialVersionUID = 1L;
+ };
+
+ /** The eight shapes the flexible state of a component can take. */
+ public enum Shape
+ {
+ NONE(false, false, false),
+ MODEL(true, false, false),
+ BEHAVIOR(false, true, false),
+ METADATA(false, false, true),
+ MODEL_BEHAVIOR(true, true, false),
+ MODEL_METADATA(true, false, true),
+ BEHAVIOR_METADATA(false, true, true),
+ MODEL_BEHAVIOR_METADATA(true, true, true),
+ /**
+ * A behavior with a stable id, as every link and ajax-enabled
component has. Master keeps
+ * those ids in a {@code BehaviorIdList} held in the
component's meta data; storing the id
+ * as the behavior's own array index removes that list, which
WICKET-6774 claimed as its
+ * biggest saving. None of the other shapes exercise it.
+ */
+ STABLE_ID_BEHAVIOR(false, false, false, true),
+ MODEL_STABLE_ID_BEHAVIOR(true, false, false, true),
+ /**
+ * A real {@link AjaxEventBehavior}, so the figure is
comparable to the -36.2% serialized
+ * saving reported on WICKET-6774. {@link #STABLE_ID_BEHAVIOR}
isolates the id storage but
+ * carries almost nothing of its own, which flatters the
percentage.
+ */
+ AJAX_BEHAVIOR(false, false, false, true, true);
+
+ private final boolean model;
+ private final boolean behavior;
+ private final boolean metaData;
+ private final boolean stableId;
+ private final boolean ajax;
+
+ Shape(boolean model, boolean behavior, boolean metaData)
+ {
+ this(model, behavior, metaData, false, false);
+ }
+
+ Shape(boolean model, boolean behavior, boolean metaData,
boolean stableId)
+ {
+ this(model, behavior, metaData, stableId, false);
+ }
+
+ Shape(boolean model, boolean behavior, boolean metaData,
boolean stableId, boolean ajax)
+ {
+ this.model = model;
+ this.behavior = behavior;
+ this.metaData = metaData;
+ this.stableId = stableId;
+ this.ajax = ajax;
+ }
+
+ boolean hasModel()
+ {
+ return model;
+ }
+
+ Component newComponent(String id)
+ {
+ Component c = new WebMarkupContainer(id);
+ populate(c);
+ return c;
+ }
+
+ void populate(Component c)
Review Comment:
Maybe:
1. Make create interface IShape with a method populate(Component c)
2. Then you make Shape implement this interface and a given enum contant
implements populate
Then you don't need the booleans and each populate clearly show what it is
populating?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
