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

Switch to Opcodes.ASM5 for Java 8 support #34

Merged
merged 8 commits into from Mar 16, 2015

Conversation

Projects
None yet
3 participants
@jhaber

jhaber commented Mar 12, 2015

By default this doesn't change anything, but it allows you to pass args to the build to change the language level of the test classes. For example to run the tests in Java 8 mode you would run

mvn clean test -Djava.test.version.source=1.8 -Djava.test.version.target=1.8

(this produces 5 test failures)

Or to see if the tests pass with the new Java 8 parameter name reflection flag you would run

mvn clean test -Djava.test.version.source=1.8 -Djava.test.version.target=1.8 -Djava.test.compiler.argument=-parameters

(this produces 15 test failures)

Related to issue #33, I was going to write a small Java 8 test but I figured it would be much more robust to run the entire test suite against Java 8. Unfortunately this type of testing would have to be manual, I could try making it part of the build but the result would probably be pretty nasty.

@jhaber

This comment has been minimized.

Show comment
Hide comment
@jhaber

jhaber Mar 13, 2015

I updated the build to compile test classes twice, once as 1.5 and once as 1.8 (with -parameters flag) and to run the test phase against both. As expected its ugly but it seems to work

jhaber commented Mar 13, 2015

I updated the build to compile test classes twice, once as 1.5 and once as 1.8 (with -parameters flag) and to run the test phase against both. As expected its ugly but it seems to work

@jhaber

This comment has been minimized.

Show comment
Hide comment
@jhaber

jhaber Mar 13, 2015

Might be easier to just activate a profile based on jdk version for testing java 8

jhaber commented Mar 13, 2015

Might be easier to just activate a profile based on jdk version for testing java 8

@jhaber

This comment has been minimized.

Show comment
Hide comment
@jhaber

jhaber Mar 13, 2015

Updated to use a profile for Java 8 so the travis build fails when running on JDK8

jhaber commented Mar 13, 2015

Updated to use a profile for Java 8 so the travis build fails when running on JDK8

@sameb

This comment has been minimized.

Show comment
Hide comment
@sameb

sameb Mar 15, 2015

Contributor

Thanks for this, @HiJon89.

A few questions:

  • Are the test failures because of -parameters? Or because of other stuff?
  • First, background: When we initially added java7, the build failed with a VerifyError (see https://travis-ci.org/cglib/cglib/jobs/26752743). Ultimately, we tracked this down to the use of 'stack map frames' added in java7, which cglib doesn't create bytecode for. In #23 we realized that the ant builds failed because they compiled the code with target=1.6, but the tests didn't have a specific target set so it used the JDK's target. The maven POMs used the same target for code & test, which let the tests pass. So by setting the target to <= 1.6, java didn't care about the stack map frames, and everything was great.
    The question: The patch looks like it's setting the target to >= 1.7 for tests, so why don't I see the verify error anymore? Is java8 more lenient? Is there a new asm dependency that's better?
Contributor

sameb commented Mar 15, 2015

Thanks for this, @HiJon89.

A few questions:

  • Are the test failures because of -parameters? Or because of other stuff?
  • First, background: When we initially added java7, the build failed with a VerifyError (see https://travis-ci.org/cglib/cglib/jobs/26752743). Ultimately, we tracked this down to the use of 'stack map frames' added in java7, which cglib doesn't create bytecode for. In #23 we realized that the ant builds failed because they compiled the code with target=1.6, but the tests didn't have a specific target set so it used the JDK's target. The maven POMs used the same target for code & test, which let the tests pass. So by setting the target to <= 1.6, java didn't care about the stack map frames, and everything was great.
    The question: The patch looks like it's setting the target to >= 1.7 for tests, so why don't I see the verify error anymore? Is java8 more lenient? Is there a new asm dependency that's better?
@jhaber

This comment has been minimized.

Show comment
Hide comment
@jhaber

jhaber Mar 16, 2015

I get strictly more test failures with -parameters so it does make a difference, but I still get test failures just by compiling the tests with Java 8.

Those stack frame errors are still there, but the JDK6 and JDK7 builds still target 1.5 for test compilation so they don't manifest themselves. The JDK8 build targets 1.8 and gets the stack frame errors. It looks like there's a ClassWriter flag that will tell ASM to automatically compute the stack map frames, do you know of a reason not to pass this flag?

Also, do you want to merge this PR and have a failing travis build on master or do you want me to try to fix the tests on Java 8 as part of this PR?

jhaber commented Mar 16, 2015

I get strictly more test failures with -parameters so it does make a difference, but I still get test failures just by compiling the tests with Java 8.

Those stack frame errors are still there, but the JDK6 and JDK7 builds still target 1.5 for test compilation so they don't manifest themselves. The JDK8 build targets 1.8 and gets the stack frame errors. It looks like there's a ClassWriter flag that will tell ASM to automatically compute the stack map frames, do you know of a reason not to pass this flag?

Also, do you want to merge this PR and have a failing travis build on master or do you want me to try to fix the tests on Java 8 as part of this PR?

@sameb

This comment has been minimized.

Show comment
Hide comment
@sameb

sameb Mar 16, 2015

Contributor

If possible I'd prefer to have it all working before merging. IIRC, @raphw did some measurements and found that passing the COMPUTE_FRAMES flag measurably slowed things down (see #11 (comment)). I don't know exactly how that was measured though. If it turns out to be strictly necessary on java8, then so be it... but if there was some way we could avoid it whenever possible, that'd be much nicer.

Contributor

sameb commented Mar 16, 2015

If possible I'd prefer to have it all working before merging. IIRC, @raphw did some measurements and found that passing the COMPUTE_FRAMES flag measurably slowed things down (see #11 (comment)). I don't know exactly how that was measured though. If it turns out to be strictly necessary on java8, then so be it... but if there was some way we could avoid it whenever possible, that'd be much nicer.

@jhaber

This comment has been minimized.

Show comment
Hide comment
@jhaber

jhaber Mar 16, 2015

I wonder if that's referring to the time spent generating the classes or the performance of the generated class. If the former, I'm not sure how big of a concern it is; if you're generating classes in a tight loop you're already in trouble

jhaber commented Mar 16, 2015

I wonder if that's referring to the time spent generating the classes or the performance of the generated class. If the former, I'm not sure how big of a concern it is; if you're generating classes in a tight loop you're already in trouble

@jhaber

This comment has been minimized.

Show comment
Hide comment
@jhaber

jhaber Mar 16, 2015

The tests are passing on JDK6/7 as well as JDK8 (targeting 1.8 with -parameters flag). The changes were:

  • Pass Opcodes.ASM5 and ClassWriter.COMPUTE_FRAMES everywhere bee1096
  • Switch to non-deprecated version of MethodVisitor#visitMethodInsn that takes an additional boolean flag 5f67e9f c9b4f0d

jhaber commented Mar 16, 2015

The tests are passing on JDK6/7 as well as JDK8 (targeting 1.8 with -parameters flag). The changes were:

  • Pass Opcodes.ASM5 and ClassWriter.COMPUTE_FRAMES everywhere bee1096
  • Switch to non-deprecated version of MethodVisitor#visitMethodInsn that takes an additional boolean flag 5f67e9f c9b4f0d
@sameb

This comment has been minimized.

Show comment
Hide comment
@sameb

sameb Mar 16, 2015

Contributor

This looks great (though haven't looked at the new 'tees' commit yet.. it'd be great to get a test that would have failed w/o that change). One remaining question: do actually need COMPUTE_FRAMES everywhere? Is ASM smart enough to elide the frames in places they aren't required? For example, not every feature of cglib apparently needed the stackmap frames on java7/java8... as evidenced by frameworks like guice & mockito and such using cglib successfully on code targeted to java7+java8.

If having frames in the bytecode does slow down runtime, then it'd be better to elide the frames where-ever we're allowed to elide them.

Contributor

sameb commented Mar 16, 2015

This looks great (though haven't looked at the new 'tees' commit yet.. it'd be great to get a test that would have failed w/o that change). One remaining question: do actually need COMPUTE_FRAMES everywhere? Is ASM smart enough to elide the frames in places they aren't required? For example, not every feature of cglib apparently needed the stackmap frames on java7/java8... as evidenced by frameworks like guice & mockito and such using cglib successfully on code targeted to java7+java8.

If having frames in the bytecode does slow down runtime, then it'd be better to elide the frames where-ever we're allowed to elide them.

@jhaber

This comment has been minimized.

Show comment
Hide comment
@jhaber

jhaber Mar 16, 2015

From looking at the tees it seems like the goal is to override every method from the super class and delegate it to a pair of visitors. The ASM 5 upgrade added a bunch of methods to the superclass that weren't being overridden so it definitely seems more correct to override and delegate these as well

jhaber commented Mar 16, 2015

From looking at the tees it seems like the goal is to override every method from the super class and delegate it to a pair of visitors. The ASM 5 upgrade added a bunch of methods to the superclass that weren't being overridden so it definitely seems more correct to override and delegate these as well

@jhaber

This comment has been minimized.

Show comment
Hide comment
@jhaber

jhaber Mar 16, 2015

I'm not sure about the stack frames stuff, I'll try to run some benchmarks to see if its worth worrying about

jhaber commented Mar 16, 2015

I'm not sure about the stack frames stuff, I'll try to run some benchmarks to see if its worth worrying about

@sameb

This comment has been minimized.

Show comment
Hide comment
@sameb

sameb Mar 16, 2015

Contributor

Thanks @HiJon89!

Contributor

sameb commented Mar 16, 2015

Thanks @HiJon89!

@raphw

This comment has been minimized.

Show comment
Hide comment
@raphw

raphw Mar 16, 2015

Member

You can find a benchmark in the ASM documentation, computing the frames doubles runtime for creating a class. It is not too bad but since we could hypothetically know when we create stack maps we could implement something cleaner. However, I do not think that this should hold us off doing it the "quick" way to get a new version running.

Member

raphw commented Mar 16, 2015

You can find a benchmark in the ASM documentation, computing the frames doubles runtime for creating a class. It is not too bad but since we could hypothetically know when we create stack maps we could implement something cleaner. However, I do not think that this should hold us off doing it the "quick" way to get a new version running.

@sameb

This comment has been minimized.

Show comment
Hide comment
@sameb

sameb Mar 16, 2015

Contributor

Ok if this is only a potential slowness in creating the classes (not in using the classes), then I'm happy. It's pretty unlikely that creation would be in a critical fast path that happens frequently, since creating classes has a lot of permanent overhead.

@HiJon89, would it be hard to expand the tee test in java8 to make sure the new methods are overridden? If so, no worries. If not, it'd be great to add tests for that to make sure it doesn't regress.

Contributor

sameb commented Mar 16, 2015

Ok if this is only a potential slowness in creating the classes (not in using the classes), then I'm happy. It's pretty unlikely that creation would be in a critical fast path that happens frequently, since creating classes has a lot of permanent overhead.

@HiJon89, would it be hard to expand the tee test in java8 to make sure the new methods are overridden? If so, no worries. If not, it'd be great to add tests for that to make sure it doesn't regress.

@jhaber

This comment has been minimized.

Show comment
Hide comment
@jhaber

jhaber Mar 16, 2015

Hmm, that might require writing tests using Java 8 features which would mean you couldn't build cglib on anything less than JDK8

jhaber commented Mar 16, 2015

Hmm, that might require writing tests using Java 8 features which would mean you couldn't build cglib on anything less than JDK8

@jhaber

This comment has been minimized.

Show comment
Hide comment
@jhaber

jhaber Mar 16, 2015

I guess I could put the Java 8 tests in a separate module activated by a profile

jhaber commented Mar 16, 2015

I guess I could put the Java 8 tests in a separate module activated by a profile

@sameb

This comment has been minimized.

Show comment
Hide comment
@sameb

sameb Mar 16, 2015

Contributor

Yeah a separate profile would be best because then we can commit tests that actually use java8 language features and make sure cglib works properly over them. For example, we do this for Guice: https://github.com/google/guice/tree/master/jdk8-tests

Contributor

sameb commented Mar 16, 2015

Yeah a separate profile would be best because then we can commit tests that actually use java8 language features and make sure cglib works properly over them. For example, we do this for Guice: https://github.com/google/guice/tree/master/jdk8-tests

@jhaber

This comment has been minimized.

Show comment
Hide comment
@jhaber

jhaber Mar 16, 2015

Unless I'm missing something, it seems like ClassVisitorTee, FieldVisitorTee, and MethodVisitorTee are only used by ClassTransformerTee which is unused

jhaber commented Mar 16, 2015

Unless I'm missing something, it seems like ClassVisitorTee, FieldVisitorTee, and MethodVisitorTee are only used by ClassTransformerTee which is unused

@sameb

This comment has been minimized.

Show comment
Hide comment
@sameb

sameb Mar 16, 2015

Contributor

ClassTransformerTee is a public class so it's part of the API that others can use (AFAICT?). Although it does look like it's missing any tests. :-(

Contributor

sameb commented Mar 16, 2015

ClassTransformerTee is a public class so it's part of the API that others can use (AFAICT?). Although it does look like it's missing any tests. :-(

@jhaber

This comment has been minimized.

Show comment
Hide comment
@jhaber

jhaber Mar 16, 2015

Yeah, I was hoping to test the tee fixes indirectly by using CGLIB on a Java 8 class, but since those classes aren't used internally it wouldn't work. Do you want me to just add somewhat trivial tests that verify the methods are overridden?

jhaber commented Mar 16, 2015

Yeah, I was hoping to test the tee fixes indirectly by using CGLIB on a Java 8 class, but since those classes aren't used internally it wouldn't work. Do you want me to just add somewhat trivial tests that verify the methods are overridden?

@sameb

This comment has been minimized.

Show comment
Hide comment
@sameb

sameb Mar 16, 2015

Contributor

Nah if there's no tests already, no need in this PR to add some. If you want to do a followup that makes sure java8 language features work in general (e.g, cglib works if there's default methods on interfaces, lambdas in code, etc..), that'd be really awesome and a good step in making sure cglib really works on java8.

This PR looks good to me as-is, though, so I'll merge it in. Thanks very much!

Contributor

sameb commented Mar 16, 2015

Nah if there's no tests already, no need in this PR to add some. If you want to do a followup that makes sure java8 language features work in general (e.g, cglib works if there's default methods on interfaces, lambdas in code, etc..), that'd be really awesome and a good step in making sure cglib really works on java8.

This PR looks good to me as-is, though, so I'll merge it in. Thanks very much!

sameb added a commit that referenced this pull request Mar 16, 2015

Merge pull request #34 from HubSpot/master
Fix cglib to do better with java8 (and java7 for some features, too).

@sameb sameb merged commit 6238d92 into cglib:master Mar 16, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jhaber jhaber changed the title from Make test flags configurable to Switch to Opcodes.ASM5 for Java 8 support Aug 21, 2015

@jhaber jhaber referenced this pull request Aug 21, 2015

Closed

Release CGLIB 3.1.x? #20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment