Adding support for Jackson afterburner module

[cemcayiroglu]

Support for Jackson afterburner is added with optional class filter.
Filter can be used for incompatible classes.

[cemcayiroglu]

@electrum could you take a look? Thanks!

[electrum]

Have you tried running this in a large Presto cluster over an extended period of time? Did it cause any problems with the JVM? What are the performance benefits on real workloads?

The filtered classes configuration is worrisome. First off, it adds configuration for JSON, which we've never had before. More importantly, it implies this is needed for "incompatible classes". Do you have actual examples of those, or it is a theoretical concern? The Jackson documentation doesn't mention anything about this.

[cemcayiroglu]

@electrum thanks for the feedback!

Have you tried running this in a large Presto cluster over an extended period of time? Did it cause any problems with the JVM? What are the performance benefits on real workloads?

I ran multiple tests with production workload on production size clusters. Each run was for about 2-3 hours. The total CPU for Jackson went down from 40% to 20% on the coordinator.

The filtered classes configuration is worrisome. First off, it adds configuration for JSON, which we've never had before. More importantly, it implies this is needed for "incompatible classes". Do you have actual examples of those, or it is a theoretical concern? The Jackson documentation doesn't mention anything about this.

I removed the class filtering. There was an issue regarding to plugin class loader and the app class loader. I was getting class cast accepting since afterburner was casting a generated class loaded by the plugin class loader to a class loaded by APP class loader. I introduced class filtering earlier. But after a second look I found the a better fix. All we need to do is add "com.fasterxml.jackson.module.afterburner" to SPI package white list in PluginManager on Presto side.
I still think that we should keep the configuration to enable/disable this module.

private static final ImmutableList<String> SPI_PACKAGES = ImmutableList.<String>builder() .add("com.facebook.presto.spi.") .add("com.fasterxml.jackson.annotation.") .add("com.fasterxml.jackson.module.afterburner.") .add("io.airlift.slice.") .add("io.airlift.units.") .add("org.openjdk.jol.") .build();

[electrum]

For Presto, adding Afterburner into the plugin classloader whitelist is not the right approach. I believe you can solve this by doing AfterburnerModule.setUseValueClassLoader(false). By default Jackson does something very hacky (add this to the 20 other default behaviors we disable).

Airlift is an opinionated framework that ties together a curated set of libraries in a way that should "just work". The JSON module does not have configuration now and we don't want to add it for something like this. We would only add Afterburner if it was guaranteed to work correctly every time for all users. And even if we wanted to make this optional, configuration is not the right approach, since configuration is for end users of the application who should not be involved at this level.

If we're not 100% confident about adding this for everyone, all the time, there is an easy way to enable this for your application:

Module afterburner = new AfterburnerModule().setUseValueClassLoader(false);
jsonBinder(binder).addModuleBinding().to(afterburner);

[electrum]

Thanks for the report on the real world performance. Given the JVM serializers benchmark reports only a ~20% savings, I'm very surprised this can result in a 50% savings in a real application (assuming I'm understanding your numbers).

Did you notice any weird JVM issues due to GC, de-optimization, code cache, etc.?

[cemcayiroglu]

For Presto, adding Afterburner into the plugin classloader whitelist is not the right approach. I believe you can solve this by doing AfterburnerModule.setUseValueClassLoader(false). By default Jackson does something very hacky (add this to the 20 other default behaviors we disable).

@electrum could you elaborate why adding afterburner is not the right approach?

[electrum]

It’s only needed as a hack to work around the hack that Afterburner is doing. It’s wrong because plugins should not see classes in that package from the application class loader.