Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARTEMIS-2205 Performance improvements on AMQP and other parts #2467

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
5 participants
@clebertsuconic
Copy link
Contributor

clebertsuconic commented Dec 17, 2018

No description provided.

@clebertsuconic clebertsuconic force-pushed the clebertsuconic:amqp-PR branch from c11605e to 1b42010 Dec 18, 2018

amqpConnection.close(errorCondition);
amqpConnection.flush();
});
amqpConnection.runLater(() -> {

This comment has been minimized.

Copy link
@franz1981

franz1981 Dec 18, 2018

Contributor

Probably it is a naive question, but why 2 separate runLater calls instead of one?

This comment has been minimized.

Copy link
@clebertsuconic

clebertsuconic Dec 18, 2018

Author Contributor

@franz1981 During my work, the first one was runNow, the second Later. (I would accept better names BTW)

later I changed the first to later and forgot the other one :)

fixing it on my next ammend

@@ -159,7 +160,7 @@

private final SimpleString managementAddress;

protected final RoutingContext routingContext = new RoutingContextImpl(null);
protected final RoutingContext _routingContext;

This comment has been minimized.

Copy link
@franz1981

franz1981 Dec 18, 2018

Contributor

we can use just routingContext name, dropping _

This comment has been minimized.

Copy link
@clebertsuconic

clebertsuconic Dec 18, 2018

Author Contributor

@franz1981 I don't follow you

This comment has been minimized.

Copy link
@clebertsuconic

clebertsuconic Dec 18, 2018

Author Contributor

@franz1981 I reverted the change that's not needed any longer.

all amended.

* TODO: This could be refactored out of the main codebase but at a high cost.
* We may do it some day if we find an easy way that won't clutter the code too much.
* */
public class ExecutorNettyAdapter implements EventLoop {

This comment has been minimized.

Copy link
@franz1981

franz1981 Dec 18, 2018

Contributor

Instead of using ExecutorNettyAdapter we couldn't reuse OrderedExecutor interface adding inEventLoop() on it?
OrderedExecutor::inEventLoop() could just be == inHandler()

This comment has been minimized.

Copy link
@clebertsuconic

clebertsuconic Dec 18, 2018

Author Contributor

@franz1981 this is basically for unit testing, I did not want to invest a lot of code on it.

This comment has been minimized.

Copy link
@franz1981

franz1981 Dec 19, 2018

Contributor

I see that ExecutorNettyAdapter is used on AMQPConnectionContext: do you mean that only tests trigger ExecutorNettyAdapter to be created?

This comment has been minimized.

Copy link
@clebertsuconic

clebertsuconic Dec 19, 2018

Author Contributor

@franz1981 that's correct.

Placing it on the testsuite only would require some work that I do not want to go through.

@jbertram

This comment has been minimized.

Copy link
Contributor

jbertram commented Dec 18, 2018

I've read through this and it looks fine, although I can't say I understood it all. :)

@clebertsuconic clebertsuconic force-pushed the clebertsuconic:amqp-PR branch 2 times, most recently from 17378f5 to da9cdf2 Dec 18, 2018

@michaelandrepearce
Copy link
Contributor

michaelandrepearce left a comment

Ran out of time going through. Left some basic comments for now

try {
linkContext.close(true);
} catch (Exception e) {
log.error(e.getMessage(), e);

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Dec 19, 2018

Contributor

this should have a static logger method

}
} catch (Exception e) {
log.warn(e.getMessage(), e);
}

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Dec 19, 2018

Contributor

This should have a static logger method with a code

This comment has been minimized.

Copy link
@clebertsuconic

clebertsuconic Jan 7, 2019

Author Contributor

I'm removing the catch here. I don't think we need to use specific loggers on generic handlers like this though. it was a generic handler.. but it's being removed.

}

//return true;
//return getSender().getCredit() > 0;

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Dec 19, 2018

Contributor

Remove commented out code

}
} catch (Exception e) {
log.warn(e.getMessage(), e);

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Dec 19, 2018

Contributor

Static logger method needed

try {
sessionSPI.closeSender(brokerConsumer);
} catch (Exception e) {
log.warn(e.getMessage(), e);

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Dec 19, 2018

Contributor

Static logger method needed

if (releaseRequired) {
((NettyReadable) sendBuffer).getByteBuf().release();
}
// important todo: Error treatment

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Dec 19, 2018

Contributor

Looks like some work left todo here....

This comment has been minimized.

Copy link
@clebertsuconic

clebertsuconic Dec 19, 2018

Author Contributor

ouch... let me see

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Jan 8, 2019

Contributor

@clebertsuconic Did you look over this?

This comment has been minimized.

Copy link
@clebertsuconic

clebertsuconic Jan 8, 2019

Author Contributor

I'm working on it. I'm out today on a meeting... will be done tomorrow (Wed)

This comment has been minimized.

Copy link
@clebertsuconic

clebertsuconic Jan 9, 2019

Author Contributor

done!

@clebertsuconic clebertsuconic force-pushed the clebertsuconic:amqp-PR branch from da9cdf2 to b88d910 Dec 19, 2018

@clebertsuconic

This comment has been minimized.

Copy link
Contributor Author

clebertsuconic commented Dec 19, 2018

@franz1981 I have pulled your netty change here for correctness. (Now with everything async I had an issue and I almost came up with a similar fix..yours was better and ready) :)

@clebertsuconic clebertsuconic force-pushed the clebertsuconic:amqp-PR branch 2 times, most recently from 5f86587 to a427513 Jan 7, 2019

@clebertsuconic

This comment has been minimized.

Copy link
Contributor Author

clebertsuconic commented Jan 9, 2019

@michaelandrepearce / @franz1981 all done.

I squashed my commits. It was hard to keep them separated due to these changes being inter-related.

@clebertsuconic clebertsuconic force-pushed the clebertsuconic:amqp-PR branch 2 times, most recently from cadfbe5 to 0bc2aaf Jan 9, 2019

@clebertsuconic

This comment has been minimized.

Copy link
Contributor Author

clebertsuconic commented Jan 9, 2019

@michaelandrepearce / @franz1981 / @jbertram is this ready to be merged?

@clebertsuconic clebertsuconic force-pushed the clebertsuconic:amqp-PR branch from 0bc2aaf to 4a63c1d Jan 9, 2019

franz1981 and others added some commits Mar 3, 2018

ARTEMIS-2205 Netty is used in a more idiomatic way
This helped decreasing a lot of pressure on GC by not creating
as many runnables for each write.

Besides this helps fixing some of the issues I would have had on refactoring AMQP
over flushing writes and other asynchronous issues.
ARTEMIS-2205 Refactor AMQP Processing into Netty Thread
These improvements were also part of this task:
- Routing is now cached as much as possible.
- A new Runnable is avoided for each individual message,
  since we use the Netty executor to perform delivery

https://issues.apache.org/jira/browse/ARTEMIS-2205

@clebertsuconic clebertsuconic force-pushed the clebertsuconic:amqp-PR branch from 4a63c1d to daa1adb Jan 9, 2019

@asfgit asfgit closed this in e5e5744 Jan 10, 2019

return false;
public void requireHandler() {
if (!workerExecutor.inEventLoop()) {
new Exception("saco!!!").printStackTrace();

This comment has been minimized.

Copy link
@jdanekrh

jdanekrh Feb 14, 2019

Contributor

Leftover debug code here? (currently still on master)

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Feb 14, 2019

Contributor

Great easter egg! ;)

This comment has been minimized.

Copy link
@clebertsuconic

clebertsuconic Feb 14, 2019

Author Contributor

OMG.. it still on the codebase?

That's epic!!!!

never supposed to happen though.. but still!!!!

it's an almost bad word from PT-BR... meaning it would suck if it happened!!!!

lol

taking it out now!!!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.