blackdrag commented on code in PR #2820:
URL: https://github.com/apache/groovy/pull/2820#discussion_r3821836699
##########
src/main/java/org/apache/groovy/runtime/indy/IndyInvalidation.java:
##########
Review Comment:
The architectural relationship between `IndyInvalidation` and
`SwitchPointInvalidator` makes me wonder. The issue is not only that
`IndyInvalidation` is tightly coupled to `SwitchPointInvalidator`, but also
which of the two is supposed to be the abstraction boundary?
* If `IndyInvalidation` is a wrapper/facade around `SwitchPointInvalidator`,
then `SwitchPointInvalidator` is effectively an implementation detail of the
higher-level invalidation mechanism. In that case, it would be surprising for
`SwitchPointInvalidator` to remain independently exposed and used. Consumers
should normally go through `IndyInvalidation`, otherwise the wrapper does not
really define the abstraction boundary.
* If `SwitchPointInvalidator` is intended to be an independently usable
abstraction, then the two classes should be considered independent components
which happen to cooperate. In that case, their respective responsibilities,
lifecycle guarantees and invariants need to be clearly defined. In particular,
it should be clear which guarantees `SwitchPointInvalidator` provides on its
own and which only exist when it is managed through `IndyInvalidation`.
At the moment, the implementation seems somewhat in between these two
models. `IndyInvalidation` directly manages `SwitchPointInvalidator` instances
and depends on its lifecycle and global state, which makes the relationship
look more like a layered abstraction than two independent components. At the
same time, `SwitchPointInvalidator` is still exposed and can be used
independently.
This should be document, otherwise it is difficult to determine if the
current coupling is intentional or simply an implementation dependency that has
become externally visible.
If `SwitchPointInvalidator` can be used independently as well as through
`IndyInvalidation`, then the correctness of the overall invalidation machinery
cannot simply rely on invariants established by `IndyInvalidation`. The
independent use cases need to obey the same assumptions, or the boundary
between the two needs to enforce those assumptions.
If `SwitchPointInvalidator` is really only the low-level mechanism behind
`IndyInvalidation`, hiding it behind that abstraction would make the ownership
and lifecycle model much easier to reason about and would also leave room to
change the underlying invalidation mechanism later.
I think the important point is not simply whether the classes are coupled,
but whether the current API and usage actually match the intended abstraction
boundary. If they are one layered subsystem, that relationship should be
explicit; if they are independent abstractions, their independence and
invariants need to be justified.
##########
src/main/java/org/codehaus/groovy/reflection/ClassInfo.java:
##########
@@ -110,6 +112,29 @@ public class ClassInfo implements Finalizable {
private static final ManagedConcurrentLinkedQueue<ClassInfo>
modifiedExpandos =
new ManagedConcurrentLinkedQueue<ClassInfo>(weakBundle);
+ /**
+ * Whether {@link #globalClassValue} stores its values softly with
+ * resurrection ({@code -Dgroovy.use.classvalue=soft}, GROOVY-12281
+ * investigation prototype). Soft mode needs two cooperating pieces here:
+ * strong roots for non-reconstructible state ({@link
#nonReclaimableRoots})
+ * and per-Class indy domain continuity (see {@link #indyDomain()}).
+ */
+ private static final boolean SOFT_CLASS_VALUES =
GroovyClassValueFactory.isSoftMode();
Review Comment:
This isSoftMode() is a bit suspicious. Again it produces a tight coupling to
a certain implementation. Why is a ClassValue with SoftReferences a soft mode,
but a ManagedMap based version not?
##########
src/main/java/org/codehaus/groovy/reflection/GroovyClassValueFactory.java:
##########
@@ -33,13 +33,30 @@ class GroovyClassValueFactory {
*/
private static final String CLASSVALUE_MODE =
SystemUtil.getSystemPropertySafe("groovy.use.classvalue", "true");
+ /**
+ * GROOVY-12281 investigation prototype: whether values are stored
behind
+ * {@link java.lang.ref.SoftReference}s with resurrection semantics
+ * ({@code -Dgroovy.use.classvalue=soft}). Soft mode needs cooperation
from
+ * {@link ClassInfo} (strong roots for non-reconstructible state,
per-Class
+ * indy domain continuity), which is why it is exposed package-wide
rather
+ * than kept local to {@link #createGroovyClassValue}.
+ */
+ static boolean isSoftMode() {
+ return "soft".equalsIgnoreCase(CLASSVALUE_MODE);
Review Comment:
why not store soft mode as boolean?
##########
src/main/java/org/codehaus/groovy/reflection/ClassInfo.java:
##########
@@ -110,6 +112,29 @@ public class ClassInfo implements Finalizable {
private static final ManagedConcurrentLinkedQueue<ClassInfo>
modifiedExpandos =
new ManagedConcurrentLinkedQueue<ClassInfo>(weakBundle);
+ /**
+ * Whether {@link #globalClassValue} stores its values softly with
+ * resurrection ({@code -Dgroovy.use.classvalue=soft}, GROOVY-12281
+ * investigation prototype). Soft mode needs two cooperating pieces here:
+ * strong roots for non-reconstructible state ({@link
#nonReclaimableRoots})
+ * and per-Class indy domain continuity (see {@link #indyDomain()}).
+ */
+ private static final boolean SOFT_CLASS_VALUES =
GroovyClassValueFactory.isSoftMode();
+
+ /**
+ * Soft-mode strong roots for ClassInfos whose state could not be
+ * reconstructed if the instance were soft-collected and later recreated:
+ * an installed class-level MetaClass ({@link #setStrongMetaClass}),
+ * per-instance MetaClasses, or registry-written DGM/extension method
+ * arrays ({@link CachedClass#setNewMopMethods}/{@code addNewMopMethods}).
+ * The set itself is Groovy-loaded, so it contributes no foreign-loader
+ * pinning: everything in it dies with Groovy's own loader, which is the
+ * lifetime every ClassInfo has today under the default strong ClassValue.
+ * {@code null} unless soft mode is active.
+ */
+ private static final Set<ClassInfo> nonReclaimableRoots =
+ SOFT_CLASS_VALUES ? ConcurrentHashMap.newKeySet() : null;
Review Comment:
What happens if we always do that and not only in soft mode? This is also
not the only place. Either we have to do it, then this is missing an
abstraction in my eyes. Or we do not, well, then we should simplify the code
##########
src/main/java/org/codehaus/groovy/reflection/GroovyClassValueSoft.java:
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.codehaus.groovy.reflection;
+
+import org.apache.groovy.util.concurrent.ConcurrentReferenceHashMap;
+import org.apache.groovy.util.concurrent.ConcurrentReferenceHashMap.Option;
+import
org.apache.groovy.util.concurrent.ConcurrentReferenceHashMap.ReferenceType;
+
+import java.lang.ref.SoftReference;
+import java.util.EnumSet;
+
+/**
+ * GROOVY-12281 investigation prototype ({@code -Dgroovy.use.classvalue=soft}):
+ * keeps {@code java.lang.ClassValue}'s per-{@code Class} fast path but stores
+ * each value behind a {@link SoftReference}, so an association on an immortal
+ * platform class (for example {@code String}) no longer holds a strong chain
+ * to the value's class loader (JDK-8136353 / GROOVY-12142). The stored wrapper
+ * is a bootstrap-loaded {@code java.lang.ref.SoftReference}, and the per-Class
+ * entry references the defining {@code ClassValue} only weakly (via its
+ * {@code Version}; the map key is a bootstrap {@code Identity} object), so the
+ * only path from an immortal key to the Groovy-loaded value is softly
+ * reachable and is cleared under memory pressure once nothing else keeps the
+ * Groovy island alive.
+ * <p>
+ * Correctness relies on <em>resurrection</em>: a weak-key/weak-value side map
+ * is the identity authority. {@code computeValue} consults it first, so when
+ * the {@code ClassValue}'s soft reference has been cleared but the value is
+ * still reachable anywhere — for example captured by a linked call site — the
+ * same instance is re-associated rather than a fresh one created. A fresh
+ * instance can only be created once the old one is weakly unreachable, at
+ * which point no guard, cache or call site can still observe the old
+ * instance, so "two live generations of one association" cannot arise.
+ * <p>
+ * {@link #remove(Class)} stays a <em>hard detach</em> (the identity entry is
+ * purged too), preserving the documented undeploy semantics of
+ * {@link ClassInfo#remove(Class)}: the next {@code get} creates a fresh value
+ * even if the old one is still reachable somewhere.
+ *
+ * @param <T> the value type
+ */
+class GroovyClassValueSoft<T> implements GroovyClassValue<T> {
+
+ private final ComputeValue<T> computeValue;
+
+ /**
+ * Identity authority: weak identity keys (no key class is pinned by this
+ * map) and weak values (the map keeps nothing alive by itself; it only
+ * remembers a value for as long as something else does).
+ */
+ private final ConcurrentReferenceHashMap<Class<?>, T> canonical =
+ new ConcurrentReferenceHashMap<>(ReferenceType.WEAK,
ReferenceType.WEAK,
+ EnumSet.of(Option.IDENTITY_COMPARISONS));
+
+ /**
+ * Striped locks for the canonical-miss path. Creation must be mutually
+ * exclusive per key: without it two racing threads could each create and
+ * leak a distinct instance through the uncached fallback in
+ * {@link #get(Class)}, breaking the identity guarantee the side map
+ * exists to provide. (The map's own {@code putIfAbsent} cannot be used
+ * for this: on an entry whose collected value has not yet been purged it
+ * reports success without storing the replacement.)
+ */
+ private final Object[] creationLocks = new Object[64];
+
+ private final ClassValue<SoftReference<T>> store = new
ClassValue<SoftReference<T>>() {
+ @Override
+ protected SoftReference<T> computeValue(final Class<?> type) {
+ return new SoftReference<>(canonical(type));
Review Comment:
I actually no wonder... does the ClassInfo case really need a SoftReference
or could it be a WeakReference? The SoftReference is maybe the more "general",
"cache friendly" variant, but I have now second thoughts of if it should be
defined at this level, also considering the factory. But it could stay like
this for now
##########
src/main/java/org/codehaus/groovy/reflection/GroovyClassValueFactory.java:
##########
@@ -33,13 +33,30 @@ class GroovyClassValueFactory {
*/
private static final String CLASSVALUE_MODE =
SystemUtil.getSystemPropertySafe("groovy.use.classvalue", "true");
+ /**
+ * GROOVY-12281 investigation prototype: whether values are stored
behind
+ * {@link java.lang.ref.SoftReference}s with resurrection semantics
+ * ({@code -Dgroovy.use.classvalue=soft}). Soft mode needs cooperation
from
+ * {@link ClassInfo} (strong roots for non-reconstructible state,
per-Class
+ * indy domain continuity), which is why it is exposed package-wide
rather
+ * than kept local to {@link #createGroovyClassValue}.
+ */
+ static boolean isSoftMode() {
+ return "soft".equalsIgnoreCase(CLASSVALUE_MODE);
+ }
+
public static <T> GroovyClassValue<T>
createGroovyClassValue(ComputeValue<T> computeValue) {
- // GROOVY-12281 investigation prototype: "hybrid" routes
platform-loader keys to the
+ // GROOVY-12281 investigation prototypes: "hybrid" routes
platform-loader keys to the
// weak-key map and everything else to ClassValue, so immortal
platform keys never
- // pin the value's loader while user classes keep the per-class
fast path.
+ // pin the value's loader while user classes keep the per-class
fast path (measured,
+ // declined); "soft" keeps ClassValue for all keys but holds
values softly with
+ // resurrection, so immortal keys hold no strong chain to the
value's loader.
if ("hybrid".equalsIgnoreCase(CLASSVALUE_MODE)) {
Review Comment:
not part of the PR really, but if you do a boolean for soft mode, you could
also do one for hybrid then. Or you switch to a model where you do not use the
boolean, but a provider GroovyClassValueHybrid::new and so on. Just a thought
--
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]
