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.

Sign up for GitHub

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

Jump to bottom

Remove usage of Netty Atomic*FieldUpdater in favor of JDKs #1317

Merged
merged 1 commit into from
Dec 15, 2016
Merged

Remove usage of Netty Atomic*FieldUpdater in favor of JDKs #1317

merged 1 commit into from
Dec 15, 2016

Conversation

johnou
Copy link
Contributor

@johnou johnou commented Dec 15, 2016 •
edited
Loading

Motivation:

In later Java8 versions our Atomic*FieldUpdater are slower then the JDK implementations so we should not use ours anymore. Even worse the JDK implementations provide for example an optimized version of addAndGet(...) using intrinsics which makes it a lot faster for this use-case.

Modifications:

  • Remove methods that return Netty Atomic*FieldUpdaters.
  • Use the JDK implementations everywhere.

Result:

Faster code.

@johnou
Copy link
Contributor Author

johnou commented Dec 15, 2016

@slandelle heads up that without this async-http-client will be broken with the next release of Netty, see netty/netty#6130

@johnou
Copy link
Contributor Author

johnou commented Dec 15, 2016

Where is the 2.0 branch? 😞

@slandelle
Copy link
Contributor

heads up that without this async-http-client will be broken with the next release of Netty, see netty/netty#6130

Yeah, I was aware of that, but I was waiting for 4.1.7 to be released :)

Where is the 2.0 branch?

Not created yet, as I haven't backported anything yet since upgrading master to AHC2.1/Netty4.1. Will do.

@johnou
Copy link
Contributor Author

johnou commented Dec 15, 2016

Yeah, I was aware of that, but I was waiting for 4.1.7 to be released :)

Ow, unfortunately I am on a tight release schedule and I just cut an internal build of Netty from 4.0 branch and our application blew up unexpectedly..

java.lang.NoClassDefFoundError: Could not initialize class org.asynchttpclient.netty.NettyResponseFuture
	at org.asynchttpclient.netty.request.NettyRequestSender.newNettyResponseFuture(NettyRequestSender.java:311) ~[async-http-client-2.0.24.jar:?]
	at org.asynchttpclient.netty.request.NettyRequestSender.newNettyRequestAndResponseFuture(NettyRequestSender.java:193) ~[async-http-client-2.0.24.jar:?]

slandelle
slandelle requested changes Dec 15, 2016
View reviewed changes
client/src/main/java/org/asynchttpclient/netty/NettyResponseFuture.java
private static final AtomicIntegerFieldUpdater<NettyResponseFuture<?>> REDIRECT_COUNT_UPDATER = newAtomicIntegerFieldUpdater(NettyResponseFuture.class, "redirectCount");
private static final AtomicIntegerFieldUpdater<NettyResponseFuture<?>> CURRENT_RETRY_UPDATER = newAtomicIntegerFieldUpdater(NettyResponseFuture.class, "currentRetry");
private static final AtomicIntegerFieldUpdater<NettyResponseFuture> REDIRECT_COUNT_UPDATER = AtomicIntegerFieldUpdater.newUpdater(NettyResponseFuture.class, "redirectCount");
private static final AtomicIntegerFieldUpdater<NettyResponseFuture> CURRENT_RETRY_UPDATER = AtomicIntegerFieldUpdater.newUpdater(NettyResponseFuture.class, "currentRetry");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the <?>? Without them, you get tons of compiler warnings.

Copy link
Contributor Author

@johnou johnou Dec 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fails to compile otherwise..

Error:(51, 137) java: incompatible types: inferred type does not conform to equality constraint(s)
inferred: org.asynchttpclient.netty.NettyResponseFuture equality constraints(s): org.asynchttpclient.netty.NettyResponseFuture,org.asynchttpclient.netty.NettyResponseFuture

Copy link
Contributor Author

@johnou johnou Dec 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Netty takes Class<? super T> while JDK takes Class<U>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and for what it's worth I didn't see any compiler warnings after removing the wildcard, maybe you could confirm with your IDE too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add @SuppressWarnings("rawtypes") annotations :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add it but are you absolutely sure it's necessary? my IDE and compiler are happy with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

@slandelle
Copy link
Contributor

2.0 branch published

@johnou
Copy link
Contributor Author

johnou commented Dec 15, 2016

Thanks, I will cut an internal build of async-http-client too.

@johnou
Remove usage of Netty Atomic*FieldUpdater in favor of JDKs
42686d3 
Motivation:

In later Java8 versions our Atomic*FieldUpdater are slower then the JDK implementations so we should not use ours anymore. Even worse the JDK implementations provide for example an optimized version of addAndGet(...) using intrinsics which makes it a lot faster for this use-case.

Modifications:

- Remove methods that return Netty Atomic*FieldUpdaters.
- Use the JDK implementations everywhere.

Result:

Faster code.
@slandelle slandelle merged commit b4d93c6 into AsyncHttpClient:master Dec 15, 2016
@slandelle
Copy link
Contributor

Thanks!

@slandelle slandelle added this to the 2.1.0 milestone Dec 15, 2016
@johnou
Copy link
Contributor Author

johnou commented Dec 15, 2016

Cheers!

@johnou johnou deleted the use_jdk_updater branch December 15, 2016 16:39
slandelle pushed a commit that referenced this pull request Dec 15, 2016
@johnou @slandelle
Remove usage of Netty Atomic*FieldUpdater in favor of JDKs (#1317)
6f53cda 
Motivation:

In later Java8 versions our Atomic*FieldUpdater are slower then the JDK implementations so we should not use ours anymore. Even worse the JDK implementations provide for example an optimized version of addAndGet(...) using intrinsics which makes it a lot faster for this use-case.

Modifications:

- Remove methods that return Netty Atomic*FieldUpdaters.
- Use the JDK implementations everywhere.

Result:

Faster code.
@slandelle
Copy link
Contributor

cherry-picked on 2.0 as 6f53cda

slandelle added a commit that referenced this pull request Jan 12, 2017
@slandelle
Update missing from #1317
1dcef08
avdv added a commit to avdv/playframework that referenced this pull request Jan 16, 2017
@avdv
Upgrade Netty to version 4.0.43.Final
8d02c08 
The async-http-client needs to be upgraded too, as it used an internal Netty API
which has been removed in 4.0.43.Final, see [1].

[1]: AsyncHttpClient/async-http-client#1317
@avdv avdv mentioned this pull request Jan 16, 2017
Closed
avdv added a commit to avdv/playframework that referenced this pull request Jan 25, 2017
@avdv
Upgrade Netty to version 4.0.43.Final
2f79402 
The async-http-client needs to be upgraded too, as it used an internal Netty API
which has been removed in 4.0.43.Final, see [1].

[1]: AsyncHttpClient/async-http-client#1317
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slandelle slandelle slandelle requested changes

Assignees
No one assigned
Labels
Enhancement
Projects
None yet
Milestone
2.1.0
Development

Successfully merging this pull request may close these issues.
2 participants
@johnou @slandelle