3
\$\begingroup\$

So I have my own library to organize advanced critical sections at GitHub. It's based on the ResourceDataStorage class which is responsible for creating, storing and clearing resource records. I've tried to minimize synchronization blocks hits and I've used rehash function that differs from ConcurrentHashMap rehash to keep HashMap healthy:

package zur13.checkpoint.resource.storage;

import java.util.concurrent.ConcurrentHashMap;

import zur13.checkpoint.resource.AResourceData;
import zur13.checkpoint.resource.ResourceDataFactory;

/**
 * Provides thread safe operations to store, create and release Resource Data objects.
 * Has critical sections at get() and release() operations.
 * Synchronously clears internal records for resource if no references left on release().
 *
 */
public class ResourceDataStorage {
    ConcurrentHashMap<Object, AResourceData> dataBuckets[];
    ResourceDataFactory adf;

    @SuppressWarnings("unchecked")
    public ResourceDataStorage(ResourceDataFactory adf, int concurrencyLevel) {
        super();
        this.adf = adf;
        dataBuckets = new ConcurrentHashMap[concurrencyLevel];
        for (int i = 0; i < dataBuckets.length; i++) {
            dataBuckets[i] = new ConcurrentHashMap<Object, AResourceData>();
        }
    }

    /**
     * Retrieve ResourceData instance for the given resource.
     * Create new instance of the ResourceData if no instance stored for the given resource.
     * 
     * Release ResourceData after the passes it supplied is returned or you have done working with it.
     * 
     * @return
     */
    public AResourceData get(Object resourceId) {
        AResourceData ad = null;
        AResourceData adPrev = null;

        int bucketIdx = spread(resourceId.hashCode()) % dataBuckets.length;
        ConcurrentHashMap<Object, AResourceData> resourcesDataBucket = dataBuckets[bucketIdx];

        ad = resourcesDataBucket.get(resourceId);

        if ( ad == null || ad.getRefCounter().getAndIncrement() <= 0 ) {
            adPrev = ad;
            synchronized (resourcesDataBucket) {
                ad = resourcesDataBucket.get(resourceId);
                if ( ad == null ) {
                    ad = adf.getResourceData(resourceId); // default refCounter == 1
                    resourcesDataBucket.put(resourceId, ad);
                } else if ( ad != adPrev ) {
                    // ResourceData was recreated and put to Hash Map after refCounter.getAndIncrement()
                    // but before synchronized() block
                    ad.getRefCounter().getAndIncrement();
                }
            }
        }

        return ad;
    }

    /**
     * Release ResourceAccessController instance and clear it from the storage if no references left.
     *
     * @param resourceId
     */
    public void release(Object resourceId) {
        int bucketIdx = spread(resourceId.hashCode()) % dataBuckets.length;
        ConcurrentHashMap<Object, AResourceData> resourcesDataBucket = dataBuckets[bucketIdx];

        AResourceData ad = resourcesDataBucket.get(resourceId);

        if ( ad.getRefCounter().decrementAndGet() <= 0 ) {
            synchronized (resourcesDataBucket) {
                if ( ad.getRefCounter().get() <= 0 ) {
                    resourcesDataBucket.remove(ad.getResourceId());
                }
            }
        }
    }

    /**
     * Spread hash to minimize collisions inside ConcurrentHashMaps
     * 
     * @param h
     * @return
     */
    static final int spread(int h) {
        return (h ^ (h >> 8));
    }
}

And here is AResourceData class I've cut some things that are not important for this question

package zur13.checkpoint.resource;

import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicLong;
import zur13.checkpoint.ACheckpoint;
import zur13.checkpoint.Pass;

/**
 * Stores active passes data for a single resource.
 */
public abstract class AResourceData {
    protected Object resourceId;
    protected AtomicLong refCounter = new AtomicLong(1);

    public AResourceData() {
        super();
    }

    public Object getResourceId() {
        return this.resourceId;
    }

    public AtomicLong getRefCounter() {
        return this.refCounter;
    }

    @Override
    public int hashCode() {
        final int prime = 31;
        int result = 1;
        result = prime * result + ((this.resourceId == null) ? 0 : this.resourceId.hashCode());
        return result;
    }

    /**
     * Allows comparison with other resourceId.
     */
    @Override
    public boolean equals(Object obj) {
        if ( this == obj )
            return true;
        if ( obj == null )
            return false;

        if ( obj instanceof AResourceData ) {
            AResourceData other = (AResourceData) obj;
            if ( this.resourceId == null ) {
                if ( other.resourceId != null )
                    return false;
            } else if ( !this.resourceId.equals(other.resourceId) )
                return false;
        } else {
            // suggest that obj is resourceId
            if ( this.resourceId == null ) {
                if ( obj != null )
                    return false;
            } else if ( !this.resourceId.equals(obj) )
                return false;
        }
        return true;
    }
}

And ResourceDataFactory is pretty simple here is the only needed method from it:

public AResourceData getResourceData(Object resourceId) {
        return new ResourceData(resourceId, maxActivePassesPerResource, fair);
}
\$\endgroup\$
1
\$\begingroup\$

Mark variables as final when they should not change

public class ResourceDataStorage {
    ConcurrentHashMap<Object, AResourceData> dataBuckets[];
    ResourceDataFactory adf;

public abstract class AResourceData {
    protected Object resourceId;
    protected AtomicLong refCounter = new AtomicLong(1);

At the moment, these fields can be changed from classes in the same package, potentially breaking thread safe contract

final ConcurrentHashMap<Object, AResourceData> dataBuckets[];
final ResourceDataFactory adf;


protected final Object resourceId;
protected final AtomicLong refCounter = new AtomicLong(1);

Code breaks after more than 9,223,372,036,854,775,807 accesses to the get method of ResourceDataStorage without release

    if ( ad == null || ad.getRefCounter().getAndIncrement() <= 0 ) {

While this technically is a bug, it is unlikely to get triggered in the short term, however it might get triggered in the long term for long running applications.

NPE in your release method

   AResourceData ad = resourcesDataBucket.get(resourceId);

   if ( ad.getRefCounter().decrementAndGet() <= 0 ) {

What if that object was never requested? It would throw a NPE

Inefficiencies between your synchronization & ConcurrentHashMaps ones

Internally, a ConcurrentHashMaps uses the same object bucket system as you effectively designed, by using both of these systems at the same time, you're basically wasting time doing locking, synchronizing making multiple buckets.

I suggest just relying on the buckets stored inside ConcurrentHashMaps and dropping your own bucket system, and just passing the concurrencyLevel directly toward the map constructor.

Add null checks in constructors

public ResourceDataStorage(ResourceDataFactory adf, int concurrencyLevel) {
   super();
   this.adf = adf;

By adding a null check inside constructors, you get the surprise when making the object, and not 10 methods later when you actually use the function

 public ResourceDataStorage(ResourceDataFactory adf, int concurrencyLevel) {
    super();
    this.adf = Object.requireNonNull(adf, "adf");

Fix javadoc warnings

Inside your javadoc, there are a few warnings

/**
 * Retrieve ResourceData instance for the given resource.
 * Create new instance of the ResourceData if no instance stored for the given resource.
 * 
 * Release ResourceData after the passes it supplied is returned or you have done working with it.
 * 
 * @return
 */
public AResourceData get(Object resourceId) {
  • Missing @param for resourceId
/**
 * Allows comparison with other resourceId.
 */
@Override
public boolean equals(Object obj) {
  • Missing @param for obj
  • Missing @return

Contract violation of .equals & .hashcode

/**
 * Allows comparison with other resourceId.
 */
@Override
public boolean equals(Object obj) {
    if ( this == obj )
        return true;
    if ( obj == null )
        return false;

    if ( obj instanceof AResourceData ) {
        AResourceData other = (AResourceData) obj;
        if ( this.resourceId == null ) {
            if ( other.resourceId != null )
                return false;
        } else if ( !this.resourceId.equals(other.resourceId) )
            return false;
    } else {
        // suggest that obj is resourceId
        if ( this.resourceId == null ) {
            if ( obj != null )
                return false;
        } else if ( !this.resourceId.equals(obj) )
            return false;
    }
    return true;
}

According to the documentation of .equals, it should be:

  • It is symmetric: for any non-null reference values x and y, x.equals(y) should return true if and only if y.equals(x) returns true.

Your code fails this:

AResourceData data = new AResourceData();
String key = "hi";
data.resourceId = key;
key.equals(data); // false
data.equals(key); // true

According to the documentation of hashcode:

If two objects are equal according to the equals(Object) method, then calling the hashCode method on each of the two objects must produce the same integer result.

AResourceData data = new AResourceData();
String key = "hi";
data.resourceId = key;
data.equals(key); // true
data.hashCode() == key.hashCode(); //false

By breaking the contract of existing methods, you basically enter the under documented world, and this may give unintended side effects and potentially future bugs.

Unused imports

import java.util.concurrent.TimeUnit;
import zur13.checkpoint.ACheckpoint;
import zur13.checkpoint.Pass;

All the imports on the top are not used anywhere inside the posted code

Add generics to the code

At the moment, the code only accepts Object as key types, by adding generics to the code itself, people can re-use it more cleanly by only accepting domain specific types, this especially matters for the people writing implementations to the ResourceDataFactory.getResourceData method

\$\endgroup\$
  • \$\begingroup\$ Thanks for your review, I really missed many things. I will investigate it in deep in a few hours. But now I can't agree on the "Inefficiencies between your synchronization & ConcurrentHashMap" the main purpose of my data buckets is to minimize collisions on synchronize blocks I had to use synchronized (resourcesDataBucket) to guarantee the single creation of ResourceData object when requested simultaneously by several threads. \$\endgroup\$ – Zur13 Apr 9 '18 at 11:16
  • \$\begingroup\$ I'd also like to add that "Code breaks after more than 2,147,483,647 accesses to the get method of ResourceDataStorage" ref counter is AtomicLong and according to library contract the thread should release resource once it finished to use it so if it hits the limit this means there is a resource leak somewhere or somebody requests resource inside very very large cycle that drained Long value capacity causing overflow. \$\endgroup\$ – Zur13 Apr 9 '18 at 11:52
  • \$\begingroup\$ It's better to throw an exception, than to silencely release it as it overflows. (Notice that I was quite off on the limit, I misread it as a int instead of a long, I fixed the number in the post) \$\endgroup\$ – Ferrybig Apr 9 '18 at 11:59
  • \$\begingroup\$ Another solution for the "conflicting map feedback would be playing around with its own concurrencyLevel, I suggest you profile your code to see what numbers work the best in the most situations, or expose those values so the consumer of your api can play with those for their requested performance \$\endgroup\$ – Ferrybig Apr 9 '18 at 12:02
  • \$\begingroup\$ Just to be clear Long.MAX_VALUE is 9,223,372,036,854,775,807 if execute get method once per nanosecond, it would take over 292 years to encounter this situation. \$\endgroup\$ – Zur13 Apr 9 '18 at 12:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.