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

Implement Caliper benchmarks for running on Android devices (#10). #278

Merged
merged 6 commits into from
Aug 7, 2017

Conversation

flooey
Copy link
Contributor

@flooey flooey commented Aug 3, 2017

Netty tcnative doesn't work on ARM, so we split out the Netty-dependent
benchmark portions into benchmark-openjdk and put non-Netty versions
in benchmark-android.

The gradle operations required to execute the benchmarks on devices are ugly,
but they appear to work. This might get better some time in the future
with improved Caliper support for Android.

This only includes the time-based benchmarks, not the throughput-based ones,
since those will require some more modifications to work in Caliper and these
at least get us started.

Netty tcnative doesn't work on ARM, so we split out the Netty-dependent
benchmark portions into benchmark-openjdk and put non-Netty versions
in benchmark-android.

The gradle operations required to execute the benchmarks on devices are ugly,
but they appear to work.  This might get better some time in the future
with improved Caliper support for Android.

This only includes the time-based benchmarks, not the throughput-based ones,
since those will require some more modifications to work in Caliper and these
at least get us started.
@flooey flooey requested a review from nmittler August 3, 2017 14:38
jcenter()
}
dependencies {
classpath 'com.android.tools.build:gradle:2.3.3' // jcenter has the latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we define a version variable for this at the top-level? There are now 3 modules that use 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.

Done.

sourceSets.main {
java {
srcDirs = [
"${rootDir}/benchmark-base/src/main/java",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just make base a dependency rather than including the source?

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 removed the benchmark-base subproject, so benchmark-android and benchmark-openjdk both include its source directly. This is the same pattern used with common/, it doesn't get compiled directly but android/ and openjdk/ include its source. It makes it a bit easier because there are circular dependencies between the platform-specific bits and the platform-agnostic bits, so this just lets them get compiled together.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok that's fine. FYI the main reason we were including source in common is because we wanted a single resulting jar.

tasks.collect {
it.enabled = false
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add a new line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* Enumeration of various types of engines for use with engine-based benchmarks.
*/
@SuppressWarnings({"ImmutableEnumChecker", "unused"})
public enum EngineType {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this is being redefined because we can't pull in Netty on android? Maybe we should re-think this a bit? Perhaps EngineType should be an interface and we have a number of implementations? The Netty impl can be in a separate module so that we don't force the benchmarks to pull it in. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it needs to not have any references to Netty.

It could be an interface. The main downside I can see is that it would make the base benchmark classes unable to have the main() methods. They could be moved to the Jmh versions, though it seems a bit odd to run the Jmh class specifically not under Jmh, but not a huge deal. I think I'm fine either way, if you have a preference for the interface version I can change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't worry too much about the main methods. They're really just there for convenience and they can just as easily be rewritten whenever we need them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, swapped to an interface.

@@ -52,7 +52,7 @@
private static final AtomicLong bytesCounter = new AtomicLong();
private AtomicBoolean recording = new AtomicBoolean();

ServerSocketBenchmark(Config config) throws Exception {
ServerSocketBenchmark(final Config config) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious ... why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Android's toolchain doesn't support Java 8's "effectively final" determinations for local variables, so we have to explicitly declare it as final so it can be used inside the closure below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rather, this isn't specific to the Android toolchain, but some people might still be compiling with Java 7 (which happened in one of the various toolchains I used at some point), which doesn't support effectively final.

@@ -0,0 +1,142 @@
buildscript {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need benchmark-caliper at all? I don't think we'll ever want to use caliper on non-android .. JMH is the preferred framework.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, Android is always Caliper. Folded the stuff into benchmark-android.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok great .... do we need to delete this then, or am I looking at an outdated diff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like my merge over master lost the delete. Deleted again.

Adam Vartanian and others added 5 commits August 4, 2017 11:43
Deletes the benchmark-caliper submodule and folds it into benchmark-android,
since Caliper is the only option for running Android benchmarks.
Netty tcnative doesn't work on ARM, so we split out the Netty-dependent
benchmark portions into benchmark-openjdk and put non-Netty versions
in benchmark-android.

The gradle operations required to execute the benchmarks on devices are ugly,
but they appear to work.  This might get better some time in the future
with improved Caliper support for Android.

This only includes the time-based benchmarks, not the throughput-based ones,
since those will require some more modifications to work in Caliper and these
at least get us started.
@nmittler
Copy link
Contributor

nmittler commented Aug 4, 2017

@flooey could you post the android benchmark results here as well? It would be good to see how the new engine-based socket performs compared to the FD-based one.

@flooey
Copy link
Contributor Author

flooey commented Aug 7, 2017

These don't yet include the throughput-based benchmarks (since Caliper is oriented around time-based benchmarks), so it doesn't have the socket benchmarks. Here are the results for the three it has (measurements in ns). All tests were done on a Nexus 6P running a top-of-tree AOSP eng build.

CaliperAlpnBenchmark

DIRECT	8,704,335.738
HEAP	8,565,993.818

CaliperEngineHandshakeBenchmark

DIRECT	8,353,763.850
HEAP	8,585,778.671

CaliperEngineWrapBenchmark

timeWrap	DIRECT	4096	14,330.952
timeWrap	DIRECT	512	10,045.727
timeWrap	DIRECT	64	8,987.334
timeWrap	HEAP	4096	16,737.397
timeWrap	HEAP	512	11,916.099
timeWrap	HEAP	64	11,174.679
timeWrapAndUnwrap	DIRECT	4096	31,751.086
timeWrapAndUnwrap	DIRECT	512	22,187.506
timeWrapAndUnwrap	DIRECT	64	20,564.858
timeWrapAndUnwrap	HEAP	4096	37,393.938
timeWrapAndUnwrap	HEAP	512	24,921.011
timeWrapAndUnwrap	HEAP	64	23,184.153

@flooey flooey merged commit e56b72a into google:master Aug 7, 2017
@flooey flooey deleted the benchmarks branch August 7, 2017 10:49
nmittler pushed a commit that referenced this pull request Aug 10, 2017
)

Netty tcnative doesn't work on ARM, so we split out the Netty-dependent
benchmark portions into benchmark-openjdk and put non-Netty versions
in benchmark-android.

The gradle operations required to execute the benchmarks on devices are ugly,
but they appear to work.  This might get better some time in the future
with improved Caliper support for Android.

This only includes the time-based benchmarks, not the throughput-based ones,
since those will require some more modifications to work in Caliper and these
at least get us started.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants