AtomicReference#compareAndSet bogs down main thread, should I use synchronized?

230 Views Asked by At

I've built a collection of atomic maps, sets and lists classes using AtomicReference. I've ran into a problem where when AtomicReference#compareAndSet is called ~20 times a second there is severe lag. (AtomicSet doesn't cause any lag, but that's only because it doesn't call compareAndSet if it already contains the object)

If I comment out the method and simply add synchronized to the method, the lag goes away, but is that a valid substitute and have I over-engineered for something I didn't need to?

I've tried a bunch of different combinations of method layout where I only get the current map and create a new modified map once, then just repeatedly call compareAndSet until it returns true, still lag. I've also tried just calling AtomicReference#set once, which also lags.

Summary: My AtomicReference#compareAndSet calls bog down the main thread, and I'm looking to find out if I've completely misused AtomicReference, should be using synchronized or a fix for my code, if there is one.

For context, I'm running the following code inside a Minecraft Sponge Mixin injection. All it does is inject my java code at the bottom of the defined method (which appears to run at the Minecraft tick speed, 20 times a second):

package net.netcoding.mod.mixins;

import net.minecraft.tileentity.TileEntityHopper;
import net.minecraft.tileentity.TileEntityLockable;
import net.minecraft.util.math.BlockPos;
import net.netcoding.mod.util.Cache;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.Shadow;
import org.spongepowered.asm.mixin.injection.At;
import org.spongepowered.asm.mixin.injection.Inject;
import org.spongepowered.asm.mixin.injection.callback.CallbackInfo;

@Mixin(TileEntityHopper.class)
public abstract class MixinTileEntityHopper extends TileEntityLockable {

    @Shadow public abstract double getXPos();
    @Shadow public abstract double getZPos();
    @Shadow public abstract double getYPos();

    @Inject(
            method = "update",
            at = @At(
                    value = "TAIL"
            )
    )
    private void mod_update(CallbackInfo ci) {
        BlockPos position = new BlockPos(this.getXPos(), this.getYPos(), this.getZPos());
        Cache.STORED_HOPPERS.put(position, true); // this line calls the AtomicMap#put
    }

}

Here's the AtomicMap class in question (please take a look at the put and put2 methods):

package net.netcoding.core.api.util.concurrent.atomic;

import java.util.*;
import java.util.concurrent.atomic.AtomicReference;
import java.util.stream.Stream;

public abstract class AtomicMap<K, V, M extends AbstractMap<K, V>> extends AbstractMap<K, V> implements Iterable<Map.Entry<K, V>>, Map<K, V> {

    protected final AtomicReference<M> ref;

    /**
     * Create a new concurrent map.
     */
    protected AtomicMap(M type) {
        this.ref = new AtomicReference<>(type);
    }

    @Override
    public final void clear() {
        this.ref.get().clear();
    }

    @Override
    public final boolean containsKey(Object key) {
        return this.ref.get().containsKey(key);
    }

    @Override
    public final boolean containsValue(Object value) {
        return this.ref.get().containsValue(value);
    }

    @Override
    public final Set<Entry<K, V>> entrySet() {
        return this.ref.get().entrySet();
    }

    @Override
    public final V get(Object key) {
        return this.ref.get().get(key);
    }

    @Override
    public final V getOrDefault(Object key, V defaultValue) {
        M current = this.ref.get();
        return current.getOrDefault(key, defaultValue);
    }

    @Override
    public final boolean isEmpty() {
        return this.ref.get().isEmpty();
    }

    @Override
    public Iterator<Entry<K, V>> iterator() {
        return this.entrySet().iterator();
    }

    @Override
    public final Set<K> keySet() {
        return this.ref.get().keySet();
    }

    @SuppressWarnings("unchecked")
    private M newMap(M current) {
        try {
            Map<K, V> map = current.getClass().newInstance();
            map.putAll(current);
            return (M)map;
        } catch (Exception ex) {
            throw new RuntimeException("Unable to create new map instance of " + current.getClass().getSimpleName() + "!");
        }
    }

    public final Stream<Entry<K, V>> parallelStream() {
        return this.entrySet().parallelStream();
    }

    @Override
    public final V put(K key, V value) {
        while (true) {
            M current = this.ref.get();
            M modified = this.newMap(current);
            V old = modified.put(key, value);

            // Causes severe lag if called ~20 times a second
            if (this.ref.compareAndSet(current, modified))
                return old;
        }
    }

    // Causes no lag, but answers in questions about AtomicReference and synchronized
    // all say synchronized can deadlock a thread, and hang until it's complete
    // which is actually the exact opposite of what I am experiencing
    // and I'm not sure this is even a correct way to use AtomicReference
    public synchronized final V put2(K key, V value) {
        return this.ref.get().put(key, value);
    }

    @Override
    public synchronized final void putAll(Map<? extends K, ? extends V> map) {
        this.ref.get().putAll(map);
        /*while (true) {
            M current = this.ref.get();
            M modified = this.newMap(current);
            modified.putAll(map);

            if (this.ref.compareAndSet(current, modified))
                return;
        }*/
    }

    @Override
    public synchronized final V putIfAbsent(K key, V value) {
        return this.ref.get().putIfAbsent(key, value);
        /*while (true) {
            M current = this.ref.get();
            M modified = this.newMap(current);

            if (!modified.containsKey(key) || modified.get(key) == null) {
                V old = modified.put(key, value);

                if (this.ref.compareAndSet(current, modified))
                    return old;
            } else
                return null;
        }*/
    }

    @Override
    public final V remove(Object key) {
        while (true) {
            M current = this.ref.get();

            if (!current.containsKey(key))
                return null;

            M modified = this.newMap(current);
            V old = modified.remove(key);

            if (this.ref.compareAndSet(current, modified))
                return old;
        }
    }

    @Override
    public final boolean remove(Object key, Object value) {
        while (true) {
            M current = this.ref.get();

            if (!current.containsKey(key))
                return false;

            M modified = this.newMap(current);
            V currentValue = modified.get(key);

            if (Objects.equals(currentValue, value)) {
                modified.remove(key);

                if (this.ref.compareAndSet(current, modified))
                    return true;
            } else
                return false;
        }
    }

    @Override
    public final int size() {
        return this.ref.get().size();
    }

    public final Stream<Entry<K, V>> stream() {
        return this.entrySet().stream();
    }

    @Override
    public final Collection<V> values() {
        return this.ref.get().values();
    }

}

ConcurrentMap (implementation of AtomicMap)

package net.netcoding.core.api.util.concurrent;

import net.netcoding.core.api.util.concurrent.atomic.AtomicMap;

import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.atomic.AtomicReference;

public class ConcurrentMap<K, V> extends AtomicMap<K, V, HashMap<K, V>> {

    /**
     * Create a new concurrent map.
     */
    public ConcurrentMap() {
        super(new HashMap<>());
    }

    /**
     * Create a new concurrent map and fill it with the given map.
     */
    public ConcurrentMap(Map<? extends K, ? extends V> map) {
        super(new HashMap<>(map));
    }

}
0

There are 0 best solutions below