1
\$\begingroup\$

I came across a situation where I needed code to run every n invocations of a method. Specifically, I was clearing out massive amounts of data and needed to hint to the VM that it should run garbage collection. My solution was a little more condensed than this, but this class offers reuseability and ensures accurate counts.

public class ExecutionLimiter {
    private final AtomicInteger ai = new AtomicInteger(0);
    private final Runnable task;
    private final int runEvery;

    public ExecutionLimiter(Runnable task, int runEvery){
        this.task = task;
        this.runEvery = runEvery;
    }

    public boolean tryRun(){
        synchronized(ai){
            if(ai.incrementAndGet() != runEvery) return false;
            ai.set(0);
        }
        task.run();
        return true;
    }

    public static void main(String... args) throws InterruptedException{
        AtomicInteger example = new AtomicInteger(0);
        ExecutorService service = Executors.newWorkStealingPool(5);
        ExecutionLimiter limiter = new ExecutionLimiter(example::incrementAndGet, 10);

        int invocations = 100;

        for(int i=0;i<invocations;i++){
            service.submit(()->{
                try{
                    Thread.sleep(Math.round(Math.random() * 1000));
                }catch(InterruptedException ignore){}
                if(limiter.tryRun())
                    System.out.println(Thread.currentThread().getName() +
                            ": Successfully ran: value is " + example.get());
            });
        }

        service.shutdown();
        service.awaitTermination(invocations, TimeUnit.SECONDS);
    }
}

I can't help but feel that something like this exists already in Java, but I haven't been able to find it. The closest I've come is CyclicBarrier, but I don't want to hang my threads while incrementing.

A few concerns:

  • Is AtomicInteger overkill for this class? Would it be better to use an int and synchronize on a lock Object?
  • Should I be synchronizing AtomicInteger to perform the incrementAndGet-and-set, or is there a better way to go about this?

The main is just for demonstration.

\$\endgroup\$
0

2 Answers 2

1
\$\begingroup\$
        synchronized(ai){
            if(ai.incrementAndGet() != runEvery) return false;
            ai.set(0);
        }

You should either use AtomicInteger or use synchronized. Using both together is certainly overkill.

Consider

    private static int count = 0;
    private static final Object lock = new Object();

and later

        synchronized (lock) {
            if (++count != runEvery) {
                return false;
            }

            count = 0;
        }

Then you don't need the AtomicInteger at all.

I prefer a descriptive name like count to an abbreviated type name like ai. This is especially so in this case, as my natural expansion of AI is Artificial Intelligence.

You use AtomicInteger when you don't want to use a synchronized block. Perhaps

        int current = ai.incrementAndGet();
        if (current % runEvery != 0) {
            return false;
        }

        int next = 0;
        do {
            if (ai.compareAndSet(current, next) {
                task.run();
                return true;
            }
            current = ai.get();
            next = current - runEvery;
        } while (next >= 0);

Obviously this is more complicated than the synchronized version. But this is how one uses an AtomicInteger.

The modulus is more reliable in case of high contention that keeps compareAndSet from returning true.

If we aren't worried that the count will overflow the bounds of integer, we can get rid of the reset. Without the reset, this makes more sense:

        if (ai.incrementAndGet() % runEvery != 0) {
            return false;
        }

That's how an AtomicInteger is supposed to work. Even if two threads call this code at the same time, only one will get past the check. Because the update happens atomically. The problem with the original code is that there are actually two updates and you need to synchronize them.

Of course, if the range for ai is not constrained, then it will overflow eventually. The synchronized block is more reliable in that case. This usage fits synchronized better than atomic types because there's no atomic increment, compare, and reset operation.

You could also use an explicit Lock here, but you don't seem to need it. The blocking behavior of synchronized better matches what you are doing. The lock would be better if it were OK to only sometimes update (when you can get the lock) and sometimes not (when you can't get the lock). But that's not what you are doing here.

It would be possible to create a new class with the necessary method (countCompareReset), but it may be simpler just to use the lock variable with the synchronization block.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the detail! My original implementation used modulus, but I was worried about overflow, so instead of using an int like a good little developer, I synchronized on two operations. I always forget about the boxed classes; synchronizing on Integer should make things go much more smoothly! Also, ai is an artifact of IDE autonaming, sorry! \$\endgroup\$
    – ndm13
    Jul 10, 2017 at 21:30
  • \$\begingroup\$ Tried implementing this and noticed a pretty big flaw. Integer can't be declared as final due to operations being unboxed (assign the Integer to a new Integer resulting from an int operation), therefore cannot be synchronized on reliably. Without using something with in-built methods like AtomicInteger or BigInteger, you need a final lock object, which is what I ended up doing. \$\endgroup\$
    – ndm13
    Jul 11, 2017 at 16:16
0
\$\begingroup\$

Thanks for sharing your code!

But this code has some issues:

exaggerated synchronization

Is AtomicInteger overkill for this class? Would it be better to use an int and synchronize on a lock Object?

Yes.

The code that must run uninterrupted is the increment, the compare and the reset of the counter value. Therefore you need the synchronized block.

The use of AtomicInteger is exaggerated synchronization which may slow down your application and (in more complex situations) even cause dead locks.

So always carefully choose your *synchronization scope" as you would do with "transaction scope" when talking to a databases...

tell, don't ask!

Your tryRun() method returns a boolean to tell the caller if the execution was successful or not. For what reason?

The caller cold place the "successful" indication into the task to run so that it does not need to wait for the task to finish (which makes concurrent execution kind of meaningless...).

\$\endgroup\$
3
  • \$\begingroup\$ I see your first point, but the second point mirrors the behavior of Spliterator#tryAccept(). I suppose I could asynchronously run the task, but that means spawning another thread unless I complicate things with an ExecutorService. \$\endgroup\$
    – ndm13
    Jul 10, 2017 at 16:10
  • \$\begingroup\$ @ndm13 there is no method Spliterator#tryAccept(), the only method returning abolean is Spliterator#hasCharacteristics(int characteristics). Furthermore: the Spliterator interface has nothing to do with concurrency. \$\endgroup\$ Jul 10, 2017 at 16:24
  • \$\begingroup\$ tryAdvance; sorry, I'm on mobile. docs.oracle.com/javase/8/docs/api/java/util/… And I don't understand how the action invocation taking longer than the inaction invocation makes concurrency "meaningless." \$\endgroup\$
    – ndm13
    Jul 10, 2017 at 16:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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