INT-2935: Improve `AELMP` performance #750

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
2 participants
Member

artembilan commented Feb 18, 2013

Before ApplicationEventListeningMessageProducer accepted all ApplicationEvent's and than filtered them.
It caused some ApplicationEventMulticaster.retrieverCache overhead.

  • Improve ApplicationEventListeningMessageProducer to implements SmartApplicationListener.
    It allows to make filtering earlier on first appropriate ApplicationEvent from ApplicationEventListeningMessageProducer#supportsEventType
    and caching the ApplicationListener only for that ApplicationEvent.
  • Re-register ApplicationEventListeningMessageProducer in the ApplicationEventMulticaster on ApplicationEventListeningMessageProducer#setEventTypes
    to clear ApplicationEventMulticaster.retrieverCache and make filtering on next the ApplicationEvent.
  • Move org.springframework.integration.gemfire.inbound.SpelMessageProducerSupport to core ExpressionMessageProducerSupport as useful component.
  • Add test about new logic of ApplicationEventListeningMessageProducer and its behavior with respect to the ApplicationEventMulticaster.retrieverCache.

JIRA: https://jira.springsource.org/browse/INT-2935

...ringframework/integration/endpoint/ExpressionMessageProducerSupport.java
@@ -18,21 +18,22 @@
/**
* @author David Turanski
@garyrussell

garyrussell Feb 20, 2013

Member

Needs Class JavaDoc (even if brief).

...ringframework/integration/endpoint/ExpressionMessageProducerSupport.java
@@ -41,7 +42,7 @@ public void setPayloadExpression(String payloadExpression) {
this.payloadExpression = this.parser.parseExpression(payloadExpression);
}
}
-
+
protected Object evaluationResult(Object payload){
@garyrussell

garyrussell Feb 20, 2013

Member

This method name should be an adjective (not a noun); such as evaluate, evaluateResult.

.../integration/event/inbound/ApplicationEventListeningMessageProducer.java
- if (CollectionUtils.isEmpty(this.eventTypes)) {
- this.sendEventAsMessage(event);
- return;
+ if (event instanceof ApplicationContextEvent || this.isRunning()) {
@garyrussell

garyrussell Feb 20, 2013

Member

suggest if (this.isRunning() || event instanceof ApplicationContextEvent) { so we take the early exit most of the time.

.../integration/event/inbound/ApplicationEventListeningMessageProducer.java
*/
- @SuppressWarnings("unchecked")
- public void setEventTypes(Class<? extends ApplicationEvent>[] eventTypes) {
+ public void setEventTypes(Class<? extends ApplicationEvent>... eventTypes) {
Assert.notEmpty(eventTypes, "at least one event type is required");
synchronized (this.eventTypes) {
this.eventTypes.clear();
@garyrussell

garyrussell Feb 20, 2013

Member

This is not a new problem but I am concerned about losing events (or emitting unexpected events if an event appears between the clear and the first add in addAll()).

We either need to atomically replace this.eventTypes (change it to volatile), or add a barrier to onApplicationEvent to prevent it from proceeding while this.eventTypes is being rebuilt. However, we shouldn't always synchronize onApplicationEvent; only while the rebuild is happening.

Member

artembilan commented Feb 20, 2013

Pusshed commit (for review) about ReadWriteLock in the ApplicationEventListeningMessageProducer

+
+ this.readEventTypesLock.lock();
+ try {
+ if(!this.supportsEventType(event.getClass())) {
@artembilan

artembilan Feb 21, 2013

Member

I don't like this code, but I can't invent how to do waiting for release of 'write task' ... :-(. ReadLock doesn't support Condition

@artembilan

artembilan Feb 21, 2013

Member

I've found more strange code:

if(this.readEventTypesLock.tryLock()) {
    this.readEventTypesLock.unlock();
}
else {
    this.writeEventTypesLock.lock();
    try {
        if(!this.supportsEventType(event.getClass())) {
            return;
        }
    }
    finally {
        this.writeEventTypesLock.unlock();
    }
}

but to me it seems less overhead to this.supportsEventType.
Sorry, I'm not still well with Java Concurrency :-)

Member

artembilan commented Feb 24, 2013

Pushed new commit.

So, I've changed my mind and it works like this:
*eventTypes was changed to volatole array
*there is no reason to block setEventTypes when onApplicationEvent works
*setEventTypes switch on a volatile flag eventTypesChanging, who is checked on each event in the onApplicationEvent
*if it is true received event waits a changeEventTypesLatch, who is released on the changeEventTypesLock.unlock() in the finally of setEventTypes
*after that supportsEventType is called on expectant event, to check, if it is appropriate for changed eventTypes at runtime

Added concurrency test.

A question: how about to change AbstractEndpoint#lifecycleLock to the ReentrantReadWriteLock? There is a block for each Thread on the call of this.isRunning(). That's why I made use of it like this:

if (event instanceof ApplicationContextEvent || this.isRunning()) {

where we skip unnecessary lock.

P.S. Although AbstractApplicationEventMulticaster#getApplicationListeners has synchronized (this.defaultRetriever) { there should be an additinal double check this.retrieverCache.get(cacheKey) in that block, as it is recommended for Singleton Pattern. WDYT: is there any reason to disturb Chris or even Juergen?

Member

garyrussell commented Feb 25, 2013

Well, once you switch to a volatile field, all the complexity goes away; I don't think you don't need any locks at all.

this.eventTypes = eventTypes

is an atomic operation; a thread will either see the old or new list and never a partially constructed one.

Regarding isRunning() - maybe it's easier to go back to using the local active flag like before - then you don't need a lock (reentrant or not).

Member

artembilan commented Feb 26, 2013

Hi, Gary!
Sorry for late response

you don't need any locks at all

So, do you mean remove any locking from setEventTypes and even don't use synchronized in the method declaration? I'm not sure in the atomicity of logic in this method...

And yet: why don't use ReentrantReadWriteLock for SmartLifecycle operations?

Member

garyrussell commented Feb 26, 2013

Right - with a volatile field, I don't believe we need any locks at all; setEventTypes does two things...

  1. Atomically replaces the field (this.eventTypes = newEventTypes).
  2. Updates the multicaster (clearing the cache).

if a new even arrives between 1 and 2...

  • if the event was previously handled by this adapter (but no longer is), it will be filtered out during the iteration. Result - new settings for this event
  • if the event was NOT previously handled by this adapter (but now is), we won't be called. Result - old settings for this event

While a thread is in setEventTypes a new event can arrive before 1, between 1 and 2, or after 2. In each case, it's simply a race condition as to whether this event will use the old settings, or the new settings, but there is never any inconsistency (such as that which can occur now, while changing the set in-place).

However, this comes at the "expense" of making the field volatile. However, I submit that if we were worried about the cost of volatile variables, we'd have more than just this one to worry about.

Finally, I have not looked at the performance characteristics of ReentrantLock compared with ReentrantReadWriteLock, but I submit that a local 'active' boolean (such as is used today), is much cheaper than either one.

Member

artembilan commented Feb 26, 2013

Got it, thanks.
I worry more about changeEventTypesLatch: an event is waiting one instance, but another 'write'-Thread creates new instance of CountDownLatch... With Java 7 Phaser it will be OK, I think...

About active: here it's OK. There is no any additional 'lifecycle' logic, so, all lifecycle operations are fully atomic.
I worry more about other AbstractEndpoint implementations, where start/stop have a cost before they change 'running' flag. So, here we really should be inside 'lock' with isRunning(). But if my application 'eats' a lot of Messages, I'll get benefits from ReentrantReadWriteLock. Of course, this is not the topic of this PR...

Member

artembilan commented Mar 4, 2013

HI!
Sorry for big delay.
So, I've change my mind again:

  • Revert active flag. However a question about ReentrantReadWriteLock is open. E.g. HTTP inbound has an ability of Smart Lifecycle, but it doesn't react to the change of running after stop()
  • Remove lock from AELMP
  • Add info on discarding event during eventTypes change.
  • Polishing concurrency test
Member

artembilan commented Apr 5, 2013

Rabased, polished and added avoiding 'eventTypes' mutation afterwards

Member

garyrussell commented May 6, 2013

Hi Artem, sorry it took so long to get back to this.

Looks good, except, I think we can remove the CountDownLatch and eventTypesChanging boolean altogether. They really add no value.

if (!this.supportsEventType(event.getClass())) then we can simply assume that the event was received while the supported events was being changed. See my last comment above that shows the race condition will never cause a problem, so no latch is needed. Worst case is we'll see an event that would not have been emitted after the change.

You could argue that the latch is needed to avoid the call to supportsEventType unless we know the event list is changing. However, my point is, "who cares whether the event that was recently supported is still supported?". In other words, if we receive an event because the cache was not yet updated, simply emit the event anyway (if it had arrived a few milliseconds earlier it would have been emitted anyway". So, I think, all we need is

public void setEventTypes(Class<? extends ApplicationEvent>... eventTypes) {
    this.eventTypes = new HashSet<Class<? extends ApplicationEvent>>(Arrays.asList(eventTypes));
    if (this.applicationEventMulticaster != null) {
        this.applicationEventMulticaster.addApplicationListener(this);
    }
}

public void onApplicationEvent(ApplicationEvent event) {
    if (this.active || event instanceof ApplicationContextEvent) {
        if (event.getSource() instanceof Message<?>) {
            this.sendMessage((Message<?>) event.getSource());
        }
        else {
            Object payload = this.evaluatePayloadExpression(event);
            this.sendMessage(MessageBuilder.withPayload(payload).build());
        }
    }
}
@@ -87,11 +90,12 @@ public void validateEventParserWithEventTypes() {
Assert.assertTrue(adapter instanceof ApplicationEventListeningMessageProducer);
DirectFieldAccessor adapterAccessor = new DirectFieldAccessor(adapter);
Assert.assertEquals(context.getBean("inputFiltered"), adapterAccessor.getPropertyValue("outputChannel"));
- Set<Class<? extends ApplicationEvent>> eventTypes = (Set<Class<? extends ApplicationEvent>>) adapterAccessor.getPropertyValue("eventTypes");
+ Class<? extends ApplicationEvent>[] eventTypes = (Class<? extends ApplicationEvent>[]) adapterAccessor.getPropertyValue("eventTypes");
@garyrussell

garyrussell May 6, 2013

Member

This no longer runs - eventTypes is back to being a Set.

Member

artembilan commented May 6, 2013

Hi, Gary!
Sorry for delay on all open questions: I was out of computer.
Here: I agree and I'll answer soon by commit with your concerns.

Member

artembilan commented May 6, 2013

Rebased and pushed:

the event that was recently supported is still supported

was an argument :-)
Thanks

+ public void setEventTypes(Class<? extends ApplicationEvent>... eventTypes) {
+ this.eventTypes = new HashSet<Class<? extends ApplicationEvent>>(Arrays.asList(eventTypes));
+ if (this.applicationEventMulticaster != null) {
+ this.applicationEventMulticaster.addApplicationListener(this);
}
@garyrussell

garyrussell May 7, 2013

Member

One last comment 😄 (I hope!)

I wonder if we should detect eventTypes.length == 1 && eventTypes[0] == null and reset the field to null in that case? (just like if we had never set an event type list).

Of course, they can always set it to ApplicationEvent to get all events.

Also, aside from the only element being null, maybe an Assert.noNullElements() ??

@artembilan

artembilan May 8, 2013

Member

Gery, it's very serious catch:

adapter.setEventTypes((Class<? extends ApplicationEvent>) null);

ends up with NPE now.
So, will be fixed soon.

artembilan added some commits Feb 18, 2013

INT-2935: Improve `AELMP` performance
Before `ApplicationEventListeningMessageProducer` accepted all `ApplicationEvent`'s and than filtered them.
It caused some `ApplicationEventMulticaster.retrieverCache` overhead.

* Improve `ApplicationEventListeningMessageProducer` to `implements SmartApplicationListener`.
It allows to make filtering earlier on first appropriate `ApplicationEvent` from `ApplicationEventListeningMessageProducer#supportsEventType`
 and caching the `ApplicationListener` only for that `ApplicationEvent`.
* Re-register `ApplicationEventListeningMessageProducer` in the `ApplicationEventMulticaster` on `ApplicationEventListeningMessageProducer#setEventTypes`
to clear `ApplicationEventMulticaster.retrieverCache` and make filtering on next the `ApplicationEvent`.
* Move `org.springframework.integration.gemfire.inbound.SpelMessageProducerSupport` to core `ExpressionMessageProducerSupport` as useful component.
* Add test about new logic of `ApplicationEventListeningMessageProducer` and its behavior with respect to the `ApplicationEventMulticaster.retrieverCache`.

JIRA: https://jira.springsource.org/browse/INT-2935
Member

artembilan commented May 8, 2013

Pushed NPE fix

Member

garyrussell commented May 8, 2013

LGTM; merging.

garyrussell added a commit that referenced this pull request May 8, 2013

Merge pull request #750 from artembilan/INT-2935
* INT-2935:
  INT-2935: Improve Event Inbound Adapter

@garyrussell garyrussell closed this May 8, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment