Remove CGLIB

[felixbarny]

In this issue I want to discuss the pros and cons to remove CGLIB and to base p6spy on manual wrapper objects for the main java.sql interfaces.

Pros:

• Performance
  • CGLIB was the cause of a major performance problem (see CGLIB lock contention/deadlock #325). Although the latest version of CGLIB seems to handle concurrency better there can still be contention as the synchronized blocks are not removed completely.
  • Right now, every time a JDBC call is about to happen, multiple classes have to be created - even if the proxy has already been generated for the underlying class.
  • By using manual wrappers there is no runtime overhead for creating proxies and multithreading does not harm the performance
• Plays nicely with the event architecture discussed in Add support for events #319
• Smaller jar. The majority of the p6spy jar consists of the shaded cglib dependency. Without it, p6spy would be more lightweight.
• Improved maintainability
  • Problems related to byte code manipulation are usually harder to debug and to resolve.
  • Less magical code. I'd consider CGLIB/byte code manipulation as a advanced topic. Thus it's harder for new commiters to understand the codebase. The new approach would be 100% Java SE
  • No danger of ClassNotFoundExceptions because of class loader issues

Cons:

• Casting the Connection to a vendor specific class would fail. But unwraping the connection would still work and is preferred anyway.
• If the java.sql interfaces are extended, p6spy would have to support the new methods in order to work with the latest Java versions. Adding the new methods wouldn't be a lot of hassle though.

I'm currently working on this and would appreciate feedback:

https://github.com/felixbarny/p6spy/commits/event-emitting-wrapper

[typekpb]

@felixbarny thanks for not giving up on us :)

For me the main questions would be:

• if we go for the approach, would we need to release jdk-version specific releases? As jdk 7/8/xyz might have different API we'd need to wrap, as that might require more work on project in the future
• would the common app servers work with this approach (see: https://github.com/p6spy/p6spy-it)? In fact if it could bring us websphere compatibility (which I still don't fully understand), this could be for really powerful argument, as currently we're blocked in the area: p6spy no longer works with WebSphere #186

Btw: I believe that performance can be one of the selling arguments for people using/not using p6spy, as it heavily impacts most of the traditional JEE apps (we could even do some testing compared to current situation: https://github.com/p6spy/p6spy-perf)

If someone (yes I mean you @felixbarny :)) would be able to implement also event architecture in one shot with this, world could be perfect again and each our day brighter (that's what personally convinced of :).

anyway, I'd wait for @quintonm opinion on this.

[felixbarny]

I don't think that we need different releases, but I'm not entirely sure about this. The idea is to wrap all methods of the current JDK but compile wich source and target version 1.6. As P6Core.wrapConnection returns the interface and not the wrapper implementation, the new methods can't be called from the client and we should be good.

I don't know if it would fix #186, would need more investigation.

What's the current performance impact?

This is how the logging module would look like with this approach: https://github.com/felixbarny/p6spy/blob/db029899079f113bdf7a5a2fe11531332f032863/src/main/java/com/p6spy/engine/logging/LoggingEventListener.java.

[felixbarny]

Regarding the different JDBC versions: Spring's Juergen Hoeller says I'm right: https://spring.io/blog/2015/04/03/how-spring-achieves-compatibility-with-java-6-7-and-8#comment-1949287437

[quintonm]

A little history might help here (from the resident archaeologist :) )

P6Spy 1.3 used wrapper classes just as you are suggesting. This caused several problems.

1. In app servers (like WebLogic) which are configured for a particular vendor's JDBC driver or datasource, a failure occurred because the app server could not access the vendor specific methods. The app server was never designed to unwrap the connections because it was configured to expect the vendor's driver or datasource. The workaround was to configure it as a generic JDBC driver which only worked if you did not use XA transactions.

To get around this issue, there was a nasty Ant task that uses could use to build new wrapper classes that extended the classes from the vendor's JDBC driver. All of that code was taken out back and shot in 2.0. :)

2. Some application code might be written to use vendor specific methods. Connection pooling has been wrapping connections for a long time and sometimes the statements as well. Since P6Spy 1.3 predates Java 1.6 (which introduced the Wrapper interface), the way that a connection and/or statement got unwrapped was specific to that connection pool library (or whatever wrapped the object). All of the modern versions should implement the Wrapper interface though.

3. Since P6Spy was compiled against Java 1.3 or 1.4 and then went dormant for a long time, even the new JDBC methods added to the JDK were missing in the wrappers. Hopefully, that will not be a problem going forward but it is something to consider.

Performance -

I would not jump to the conclusion that CGLIB is a major performance problem. Although we have not measured the performance impact of proxies vs wrapper classes, my gut feeling is that the impact is negligible. The typical database call has network latency and often physical IO on the DB server. On top of that, we have code to capture the statement, bind variables, results, etc and then emit string representations to a logging framework or text file. The cost of generating the proxy should not be enough to matter in comparison. That being said, it is worth taking a look at the performance impact of generating the proxies. Using wrapper classes will likely be faster although I am not sure there will be much of a difference.

A simple test might be written to measure the time is takes to create million objects using ProxyFactory versus creating a million of the same objects without a proxy.

Simplicity -

Certainly wrapper classes are easier to deal with. That brings numerous benefits. I have the same issues with following the proxy code and I am the original author of that code. ;) Even if we do not move to wrapper classes, the proxy code needs to be simplified. The class loading issue are also something that needs to be dealt with.

Different artifacts to support different JDK versions -

@felixbarny is right. We only need to compile against the highest version and set bytecode compatibility to the lowest version of Java that we will support.

Summary -

I would think that ease of use for the users of P6Spy is more important than the simplicity of the P6Spy code base. People should be able to add P6Spy without any code changes. Technically, you should unwrap JDBC objects if you expect them to implement a certain interface. However, as a developer, I tend to try it the simple way first (not unwrap) unless I need to.

BTW - My biggest concern is the impact on app servers which is also the most difficult to test.

That being said, if we have a way to switch between wrappers and proxies, we might get the best of both worlds.

[quintonm]

Just to be clear, I am not against removing CGLIB. It has been a bit of a pain although that might be partly due to how we are using it. The benefit that it gives us is that it makes P6Spy invisible for the most part due to the dynamic proxies. If we go back to wrappers (and only wrappers) we would need to come up with solutions to the problems that they caused in 1.3.

[felixbarny]

Are the app server problems covered by the integration tests? When an application is using connection pooling, do you think this problems can occur as well?

It's not only the raw proxy creation I'm concerned about, but mainly the concurrency issues. But we'd have to measure wrappers vs cglib.

Mayve you are right and the way to go is to use both approaches. Using the event architecture there won't be a lot of duplication anyway.

We could also take a look at Byte Buddy which should be faster than cglib. I've recently migrated stagemonitor to this lib with great success.

We'd need a good perf testing suite which also includes concurrency. @typekpb does your performance test provide this?

[quintonm]

We have some integration tests with app servers but they are very basic. They basically just start a web app, perform a few queries, and verify that spy.log has the expected results. @typekb would have a better feel for if they would catch this type of issue. The tests are in p6spy-it if your are interested in looking at what they do.

I think that this would occur when the app server is configured based on the database vendor. Weblogic is a good example of an app server which is likely to have a problem. When you configure a data source in the UI, you get a drop down in the database vendors. Since it is owned by Oracle, they are likely to use vendor specific method when you select the Oracle database.

I don't think that connection pools managed by applications would pose a problem. Connection pool libraries should just wrap the connections with proxies without ever needing access to anything not defined through the java.sql.* interfaces.

It is possible that some ORM frameworks might have an issue. However, I don't think that this is very likely. Given that connection pooling is the norm, ORM frameworks should be written with the assumption that connections and statements would need to be unwrapped if they need access to vendor specific methods.

[typekpb]

just a sidenote: unfortunatelly weblogic test is currently broken in the p6spy/p6spy-it repo and I didn't have a chance to fix it yet (don't think will have time for that any time soon).