It would be nice to provide some kind of "top" command where information on various properties associated with EventLoops, pausers, priorities etc. could be easily viewed and understood.

This is something that could be very useful when running almost any library that is using Threads.

TID.     TYPE              %CPU        Pauser
16422 VanillaEventLoop     100.0       BusyPauser

List of event handlers

17552 VanillaEventLoop     1.3%        LongPauser

List of event handlers

The table above is just an outline and may not be possible or even desirable as exemplified.

Maybe introduced as part of a refactor, but Timer handlers are run together with Daemon handlers (which is never if you are always busy) due to this switch case. Then consequently, the call to runTimerHandlers is empty in the hot method. Can submit a PR if you like.

We don't need two PauserMode classes

E.g. missing final and declaring volatile AtomicReference

The "threads" library is in need of documentation and code cleanups:

• What is the life-cycle of an EventHandler/EventLoop?
• When are methods invoked?
• By which threads are methods invoked?
• When does an EventHandled become eligible for scheduling?
• Is the loopFinished() guaranteed to be invoked exactly one time?
• What happens if loopFinished() throws an Exception?
• What is the purpose of EventHandler::eventLoop?
• When and why is a potential Close method invoked?
• Is the Close guaranteed to be invoked exactly one time?
• What happens if close throws an Exception?
• Can an EventHandler be added a plurality of times to an EventLoop?
• Can an EventHandler be added a plurality of times simultaneously to an EventLoop?
• Can an EventHandler be added to a plurality of EventLoops?
• Can EventLoop::addHandler block the calling thread and if so, for how long? (presently, indefinitely under some conditions)
• When can an EventHandler be added to/removed from an EventLoop?
• What is the individual order (if any) of EventHandlers within an EventLoop?
• Are there any guarantees on fairness amongst EventHandlers?
• Is the order of invocation on retained EventHandlers time-invariant ? (it is not today)
• Why is the base interface named VanillaEventHandler when it is not an implementing class?
• What is the contract when observing an EventHandler/EventLoop using another thread?

Also, test coverage should be improved, especially as most other modules rely heavily on the threads module.

Increasing the busy wait count by 4x reduced the number of times it yield/slept by a factor of 500 in one test.

Some threads that are executing LockSupport.parkNanos take a while to respond to net.openhft.chronicle.threads.Threads#shutdown(java.util.concurrent.ExecutorService)

Have added a call to unpark them to speed up their shutdown.

Was generating lots of NoSuchFieldException and also ArrayList

It is possible to create a subclass of InvalidEventHandlerException that only exists as a singleton (e.g. SingletonInvalidEventHandlerException) and that is statically created. EventHandlers that only use the InvalidEventHandlerException as a means of removing itself from the EventLoop could use the static Exception for pure removal with no underlying exception to signal. This will improve loop performance and reduce jittering.

Exceptional performance...

Upon leaving a VanillaEventLoop, the method EventHandler::loopFinished is invoked as specified in the API.

However, there is nothing in the API contract even remotely suggesting that Closeable::close should be invoked if an EventHandle incidentally happens to implement Closeable.

This might lead to highly undesirable side-effects including attempting to release native memory a plurality of times.

This could be remedied in at least two ways:

A) Add extends Closeable to the interface EventHandler and add appropriate JavaDoc defining if and when the method is invoked.

B) Remove any magic invocation of Closeable::close() from any and all implementations of the EventLoop interface.

Both A and B will have compatibility issues.

Test Case:
`
Thread t;
try (final EventLoop eventGroup = new EventGroup(false)) {
eventGroup.start();
t = new Thread(eventGroup::awaitTermination);
t.start();
eventGroup.addHandler(new EventHandler() {
@OverRide
public boolean action() throws InvalidEventHandlerException, InterruptedException {
System.out.println("time="+ System.currentTimeMillis());
return false;
}
public HandlerPriority priority() {
return HandlerPriority.MONITOR;
}

                public void eventLoop(EventLoop eventLoop){
                    System.err.println("time="+ System.currentTimeMillis());
                }
            });
    }

`
It can be found that the action method is called multiple times in a loop. Is this appropriate?
Don't understand what the purpose or role of the action method is? For example, what is its role in this project?

The interface EventLoop is not documented with JavaDocs.

The VanillaEventLoop::runLoop method basically computes a reduction of all the handlers' status and stores the result in the local variable busy. New handlers are only admitted when the reduced busy is false. In other words, as long as there is at least one handler that is busy, no new handlers are accepted.

This means that if there are long term operations (such as bootstrapping a queue subscriber), new handlers are effectively prevented from attaching in a reasonable time.

There might be other implementations of EventLoop that have the same issue. Concurrency issues must probably be considered before just admitting new handlers.

It would be useful to add a lifecycle method EventHandle::loopStarting that allows thread-specific properties to be initialized prior to any invocation of EventHandler::action. The method is invoked by the event loop thread. This would provide at least three benefits:

A) Some properties must not be concurrent (e.g. ThreadLoacal, volatile or AtomicX) as the loopStarting() is guaranteed to be invoked by the event loop thread.
B) The time-critical EventHandler::action, that is potentially called a gazillion times, could now be ridded from code that is only executed once.
C) Would provide a more logical and distinct separation of concerns.

Example from the wild (TcpHandler):

private volatile Thread actionThread;

...

    @Override
    public boolean action() throws InvalidEventHandlerException {

        ...

        // Unnecessary volatile read each invocation
        if (actionThread == null)
            actionThread = Thread.currentThread();

See also #23

Customer use case: event handler was set up as BLOCKING as it performs I/O. When it does not do anything, it is fast, so we need a pause to stop busy spinning thread

2016-11-25T00:34:49,749 [main/event-loop-monitor] [WARN |threads.MonitorEventLoop] -
java.lang.ArithmeticException: / by zero
at net.openhft.chronicle.threads.EventGroup$LoopBlockMonitor.action(EventGroup.java:226) ~[chronicle-threads-1.7.3-SNAPSHOT.jar:?]
at net.openhft.chronicle.threads.MonitorEventLoop.runHandlers(MonitorEventLoop.java:120) ~[chronicle-threads-1.7.3-SNAPSHOT.jar:?]
at net.openhft.chronicle.threads.MonitorEventLoop.run(MonitorEventLoop.java:99) ~[chronicle-threads-1.7.3-SNAPSHOT.jar:?]
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) ~[?:1.8.0_101]
at java.util.concurrent.FutureTask.run(FutureTask.java:266) ~[?:1.8.0_101]
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) [?:1.8.0_101]
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) [?:1.8.0_101]
at java.lang.Thread.run(Thread.java:745) [?:1.8.0_101]

Looks like this line (226):

long blockingInterval = blockingTimeMS / (monitoryIntervalMs / 2);

A) The API does not specify which guarantees (if any) are made to the order in which EventHandler::action is invoked across added handlers.

EventHandler objects added to a VanillaEventHandler are handled inconsistently with respect to the order in which they were added.

Define 300 tasks to be added to EventGroup. The task type is CONCURRENT. After executing close, the task is discarded directly. Warning message: "Handler in newHandlerQueue was not accepted before close net.openhft.chronicle.threads"!

Cause Analysis:
When the task is added to VanillaEventLoop and EventGroup.close() is executed, the task in newHandlerQueue is discarded directly. This is not logical!
Tasks added before close should be executed!
Isn't it waiting for the task in newHandlerQueue to complete and shutting down VanillaEventLoop?

If it is the current logic, I think its use scenario is almost non-existent. If the scenario is limited, where does the high performance performance not be realized?

source code:
private void addNewHandler(@NotNull EventHandler handler) {
HandlerPriority t1 = handler.priority();
switch (t1 == null ? HandlerPriority.MEDIUM : t1) {
case HIGH:
if (!highHandlers.contains(handler))
highHandlers.add(handler);
break;

        case REPLICATION:
        case CONCURRENT:
        case MEDIUM:
            if (!mediumHandlers.contains(handler)) {
                mediumHandlers.add(handler);
                mediumHandlersArray = mediumHandlers.toArray(NO_EVENT_HANDLERS);
            }
            break;

        case TIMER:
            if (!timerHandlers.contains(handler))
                timerHandlers.add(handler);
            break;

        case DAEMON:
            if (!daemonHandlers.contains(handler))
                daemonHandlers.add(handler);
            break;

        default:
            throw new IllegalArgumentException("Cannot add a " + handler.priority() + " task to a busy waiting thread");
    }
    handler.eventLoop(parent);
}

After adding a new handler, the following code:
handler.eventLoop(parent)
Don't know what purpose this code is based on? Can my dear friend inform this?

eventLoop is the default empty method in EventHandler, chronicle-thread does not override the eventLoop method: how does the use of handler.eventLoop(parent) be implemented?
Can my dear friend help answer it?

I really like your code on github, @peter-lawrey , and I'm always looking at you, hoping to get your point!

As part of this, deprecate net.openhft.chronicle.core.threads.EventLoop method

void addHandler(boolean dontAttemptToRunImmediatelyInCurrentThread, @NotNull EventHandler handler)

as it does not make sense to run an EventHandler in the current thread, and especially not if the event loop has not been started, or been closed.

Hey! I was just wondering if there was a performance comparison between this and other threadpool implementations, e.g. those found in java.util.concurrent.*. I'm just trying to gauge what the advantages and disadvantages of this implementation are.

Thanks!

In time-critical applications, the extra imposed check for new handles might be unacceptable so it should be possible to configure the accept modulo counter.

For backward compatibility, treat HIGH as MEDIUM.

The hash of the threads was often causing a number of consumer to run on the same thread, killing performance, This change also latest to https://github.com/ChronicleEnterprise/Chronicle-Datagrid/issues/143

A very minor issue: says "Event Group" but should say "MediumEventLoop".

2020-04-21T15:04:25.355 main/replication-acceptor-2 AcceptorEventHandler localhost80901, port=55169java.lang.IllegalStateException: Event Group has been interrupted
	at net.openhft.chronicle.threads.MediumEventLoop.checkInterrupted(MediumEventLoop.java:176)
	at net.openhft.chronicle.threads.MediumEventLoop.addHandler(MediumEventLoop.java:156)
	at net.openhft.chronicle.network.AcceptorEventHandler.action(AcceptorEventHandler.java:97)
	at net.openhft.chronicle.threads.BlockingEventLoop$Runner.run(BlockingEventLoop.java:172)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

In VanillaEventLoop we read status variables, iterate over a number of Lists and do some logic while in the inner loop whilst doing numerous volatile reads. This opens up for potential improvements:

A) Reduce the number of. volatile reads (ideally less than 1 volatile read on average per loop)

B) Pre-compile the order in which handlers are invoked

It should be noted that adding a handler is likely a relatively infrequent operation compared to actually invoking handlers. A and B are orthogonal solutions which could or could not be implemented in any particular solution.

A) Volatile read elimination

We could have ArrayLists and arrays instead of CopyOnWriteList and boolean close instead of AtomicBoolean provided that only the loop itself updates said variables.

B) Precompile

So, I was thinking about implementing an IsolatedEventLoop that "flattens" the logic so it just contains raw invocations of the handlers. This is done by unrolling logics and loops and producing an array (or perhaps even single aggregate Runnable) that depends on the currently added handlers (instances, priorities, etc).

Let's say we are adding handler h1, h2, h3, h4 to a IsolatedEventLoop. Let's further assume that h3 has HandlerPriority.HIGH and the others have HandlerPriority.MEDIUM. Upon adding h4, the CompilingEventLoop unrolls the operations:

            if (highHandlers.isEmpty()) {
                busy = runMediumLoopOnly();
            } else {
                busy = runHighAndMediumTasks();
            }

    private boolean runHighAndMediumTasks() {
        boolean busy = false;
        for (int i = 0; i < 4; i++) {
            loopStartMS = Time.currentTimeMillis();
            busy |= runAllHighHandlers();
            busy |= runOneQuarterMediumHandler(i);
        }
        return busy;
    }

    @HotMethod
    private boolean runAllHighHandlers() {
        boolean busy = false;
        for (int i = 0; i < highHandlers.size(); i++) {
            final EventHandler handler = highHandlers.get(i);
            try {
                boolean action = handler.action();
                busy |= action;
            } catch (InvalidEventHandlerException e) {
                removeHandler(handler, highHandlers);

            } catch (Throwable e) {
                Jvm.warn().on(getClass(), e);
            }
        }
        return busy;
    }

    @HotMethod
    private boolean runOneQuarterMediumHandler(int i) {
        boolean busy = false;
        final EventHandler[] mediumHandlersArray = this.mediumHandlersArray;
        for (int j = i; j < mediumHandlersArray.length; j += 4) {
            final EventHandler handler = mediumHandlersArray[j];
            try {
                busy |= handler.action();
            } catch (InvalidEventHandlerException e) {
                removeHandler(handler, mediumHandlers);
                this.mediumHandlersArray = mediumHandlers.toArray(NO_EVENT_HANDLERS);

            } catch (Throwable e) {
                Jvm.warn().on(getClass(), e);
            }
        }
        return busy;
    }

to basically:

        boolean busy = false;

       
        busy |= h3.action();
        busy |= h1.action();
        busy |= h3.action();
        busy |= h2.action();
        busy |= h3.action();
        busy |= h4.action();

Additional provisions have to be added for handling failing handlers that throw InvalidEventHandlerException but that can relatively easily be handled.

The proposed IsolatedEventLoop could potentially execute faster than a VanillaEventLoop if new handlers are never or seldom added.

/P

The EventLoop used to only remove a handler after an InvalidEventHandlerException was thrown.

Now it will remove the handler on any exception, logging a warning.
InvalidEventHandlerException will remove the handler silently as it did before.

A client reported a StreamCorrupted error and believes this is related to this issue https://stackoverflow.com/questions/51398967/how-would-comparison-cause-numerical-overflow-when-using-system-nanotime.

This happens when pauser.pause(timeout, timeunit) would throw TimeoutException BEFORE the desired timeout (15 seconds by default), and TableStoreWriterLock would forceUnlock very quickly, causing concurrent writers to corrupt the queue file.

Following code in BusyTimedPauser:

@Override
public void pause(long timeout, TimeUnit timeUnit) throws TimeoutException {
    if (time == Long.MAX_VALUE)
        time = System.nanoTime();
    if (time + timeUnit.toNanos(timeout) < System.nanoTime())
        throw new TimeoutException();
}

Should be changed to something like:

@Override
public void pause(long timeout, TimeUnit timeUnit) throws TimeoutException {
   if (time == Long.MAX_VALUE)
       time = System.nanoTime();
   if (time + timeUnit.toNanos(timeout) - System.nanoTime() < 0)
       throw new TimeoutException();
}

This explains why it happens randomly and is hard to reproduce.