Hibernate
/
hibernate-orm
mirror of https://github.com/hibernate/hibernate-orm
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

HHH-16046 Improve memory safety of mutations in EventListenerGroupImpl 

Also avoid for method listeners() to allocate a new List at each use;
 this method was deprecated but it appears it’s still being used in
 various event processors, which is being flagged as a performance issue.
This commit is contained in:
Sanne Grinovero 2023-01-14 22:19:43 +00:00 committed by Sanne Grinovero
parent 48df4e15aa
commit 9f88b56099
1 changed files with 57 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

96
hibernate-core/src/main/java/org/hibernate/event/service/internal/EventListenerGroupImpl.java
View File

@ -7,9 +7,9 @@
package org.hibernate.event.service.internal; package org.hibernate.event.service.internal;
import java.lang.reflect.Array; import java.lang.reflect.Array;
import java.util.Arrays;
import java.util.Collections; import java.util.Collections;
import java.util.LinkedHashSet; import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set; import java.util.Set;
import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionStage; import java.util.concurrent.CompletionStage;
@ -30,6 +30,9 @@ import org.hibernate.jpa.event.spi.CallbackRegistryConsumer;
import org.jboss.logging.Logger; import org.jboss.logging.Logger;
import static java.util.Arrays.asList;
import static java.util.Collections.emptyList;
/** /**
* Standard EventListenerGroup implementation * Standard EventListenerGroup implementation
* *
@ -46,8 +49,12 @@ class EventListenerGroupImpl<T> implements EventListenerGroup<T> {
private final CallbackRegistry callbackRegistry; private final CallbackRegistry callbackRegistry;
private final boolean isJpaBootstrap; private final boolean isJpaBootstrap;
private Set<DuplicationStrategy> duplicationStrategies = DEFAULT_DUPLICATION_STRATEGIES; //TODO at least the list of listeners should be made constant;
private T[] listeners = null; //unfortunately a number of external integrations rely on being able to make
//changes to listeners at runtime, so this will require some planning.
private volatile Set<DuplicationStrategy> duplicationStrategies = DEFAULT_DUPLICATION_STRATEGIES;
private volatile T[] listeners = null;
private volatile List<T> listenersAsList = emptyList();
public EventListenerGroupImpl( public EventListenerGroupImpl(
EventType<T> eventType, EventType<T> eventType,
@ -77,13 +84,26 @@ class EventListenerGroupImpl<T> implements EventListenerGroup<T> {
@Override @Override
public void clear() { public void clear() {
//Odd semantics: we're expected (for backwards compatibility) to also clear the default DuplicationStrategy. //Odd semantics: we're expected (for backwards compatibility) to also clear the default DuplicationStrategy.
duplicationStrategies = new LinkedHashSet<>();; duplicationStrategies = new LinkedHashSet<>();
listeners = null; setListeners( null );
}
// For efficiency reasons we use both a representation as List and as array;
// ensure consistency between the two fields by delegating any mutation to both
// fields to this method.
private synchronized void setListeners(T[] newListeners) {
this.listeners = newListeners;
if ( newListeners == null || newListeners.length == 0 ) {
this.listenersAsList = emptyList();
}
else {
this.listenersAsList = asList( newListeners );
}
} }
@Override @Override
public void clearListeners() { public void clearListeners() {
listeners = null; setListeners( null );
} }
@Override @Override
@ -192,27 +212,27 @@ class EventListenerGroupImpl<T> implements EventListenerGroup<T> {
private void internalAppend(T listener) { private void internalAppend(T listener) {
prepareListener( listener ); prepareListener( listener );
final T[] listenersRead = this.listeners;
final T[] listenersWrite;
if ( listeners == null ) { if ( listenersRead == null ) {
//noinspection unchecked //noinspection unchecked
this.listeners = (T[]) Array.newInstance( eventType.baseListenerInterface(), 1 ); listenersWrite = (T[]) Array.newInstance( eventType.baseListenerInterface(), 1 );
this.listeners[0] = listener; listenersWrite[0] = listener;
} }
else { else {
final int size = this.listeners.length; final int size = listenersRead.length;
//noinspection unchecked //noinspection unchecked
final T[] newCopy = (T[]) Array.newInstance( eventType.baseListenerInterface(), size+1 ); listenersWrite = (T[]) Array.newInstance( eventType.baseListenerInterface(), size+1 );
// first copy the existing listeners // first copy the existing listeners
System.arraycopy( this.listeners, 0, newCopy, 0, size ); System.arraycopy( listenersRead, 0, listenersWrite, 0, size );
// and then put the new one after them // and then put the new one after them
newCopy[size] = listener; listenersWrite[size] = listener;
this.listeners = newCopy;
} }
setListeners( listenersWrite );
} }
@Override @Override
@ -231,35 +251,38 @@ class EventListenerGroupImpl<T> implements EventListenerGroup<T> {
private void internalPrepend(T listener) { private void internalPrepend(T listener) {
prepareListener( listener ); prepareListener( listener );
final T[] listenersRead = this.listeners;
final T[] listenersWrite;
if ( this.listeners == null ) { if ( listenersRead == null ) {
//noinspection unchecked //noinspection unchecked
this.listeners = (T[]) Array.newInstance( eventType.baseListenerInterface(), 1 ); listenersWrite = (T[]) Array.newInstance( eventType.baseListenerInterface(), 1 );
this.listeners[0] = listener; listenersWrite[0] = listener;
} }
else { else {
final int size = this.listeners.length; final int size = listenersRead.length;
//noinspection unchecked //noinspection unchecked
final T[] newCopy = (T[]) Array.newInstance( eventType.baseListenerInterface(), size+1 ); listenersWrite = (T[]) Array.newInstance( eventType.baseListenerInterface(), size+1 );
// put the new one first // put the new one first
newCopy[0] = listener; listenersWrite[0] = listener;
// and copy the rest after it // and copy the rest after it
System.arraycopy( this.listeners, 0, newCopy, 1, size ); System.arraycopy( listenersRead, 0, listenersWrite, 1, size );
this.listeners = newCopy;
} }
setListeners( listenersWrite );
} }
private void handleListenerAddition(T listener, Consumer<T> additionHandler) { private void handleListenerAddition(T listener, Consumer<T> additionHandler) {
if ( listeners == null ) { final T[] listenersRead = this.listeners;
if ( listenersRead == null ) {
additionHandler.accept( listener ); additionHandler.accept( listener );
return; return;
} }
final T[] listenersWrite = (T[]) Array.newInstance( eventType.baseListenerInterface(), listenersRead.length );
System.arraycopy( listenersRead, 0, listenersWrite, 0, listenersRead.length );
final T[] localListenersRef = this.listeners;
final boolean debugEnabled = log.isDebugEnabled(); final boolean debugEnabled = log.isDebugEnabled();
for ( DuplicationStrategy strategy : duplicationStrategies ) { for ( DuplicationStrategy strategy : duplicationStrategies ) {
@ -269,8 +292,8 @@ class EventListenerGroupImpl<T> implements EventListenerGroup<T> {
// strategy's action. Control it returned immediately after applying the action // strategy's action. Control it returned immediately after applying the action
// on match - meaning no further strategies are checked... // on match - meaning no further strategies are checked...
for ( int i = 0; i < localListenersRef.length; i++ ) { for ( int i = 0; i < listenersRead.length; i++ ) {
final T existingListener = localListenersRef[i]; final T existingListener = listenersRead[i];
if ( debugEnabled ) { if ( debugEnabled ) {
log.debugf( log.debugf(
"Checking incoming listener [`%s`] for match against existing listener [`%s`]", "Checking incoming listener [`%s`] for match against existing listener [`%s`]",
@ -300,12 +323,13 @@ class EventListenerGroupImpl<T> implements EventListenerGroup<T> {
} }
prepareListener( listener ); prepareListener( listener );
listeners[i] = listener; listenersWrite[i] = listener;
} }
} }
// we've found a match - we should return // we've found a match - we should return: the match action has already been applied at this point
// - the match action has already been applied at this point // apply all pending changes:
setListeners( listenersWrite );
return; return;
} }
} }
@ -340,7 +364,6 @@ class EventListenerGroupImpl<T> implements EventListenerGroup<T> {
} }
} }
/** /**
* Implementation note: should be final for performance reasons. * Implementation note: should be final for performance reasons.
* @deprecated this is not the most efficient way for iterating the event listeners. * @deprecated this is not the most efficient way for iterating the event listeners.
@ -349,12 +372,7 @@ class EventListenerGroupImpl<T> implements EventListenerGroup<T> {
@Override @Override
@Deprecated @Deprecated
public final Iterable<T> listeners() { public final Iterable<T> listeners() {
if ( listeners == null ) { return this.listenersAsList;
//noinspection unchecked
return Collections.EMPTY_LIST;
}
return Arrays.asList( listeners );
} }
private static Set<DuplicationStrategy> makeDefaultDuplicationStrategy() { private static Set<DuplicationStrategy> makeDefaultDuplicationStrategy() {