Skip to content

Conversation

@geoand
Copy link
Contributor

@geoand geoand commented Sep 3, 2017

This PR replaces all the parts that contain ASM code-gen logic, with ByteBuddy.

I should note that while replacing the ASM code-gen code with the relevant ByteBuddy code, I tried to encapsulate the common logic and "algorithms" of the various parts into self standing classes that can be composed and reused.

Furthermore, the new code also contains a few minor changes to the generated bytecode in order to remove the following unnecessary instructions:

  • When generating code for a single method/field, the index is no longer loaded onto the stack since it's not used
  • The IllegalArgumentException is only generated when using the switch method. In the case of a single method/field or when the if method is used, the exceptions are not generated since it was impossible for control to reach them anyway.
  • The bytecode that throws the IllegalArgumentException adds the whole string to the stack and forgoes the use of StringBuilder. This is because at the time of the bytecode creation, the whole exception message is already known.

It goes without saying that I am willing to make any changes you deem necessary since I acknowledge that the surface area of the changes is rather large and that I might have missed something and/or not designed/implemented something in the optimal way.

@geoand
Copy link
Contributor Author

geoand commented Sep 3, 2017

For some odd reason it seems like TravisCI is not setting up the JDK 7 tests correctly resulting in the failure to run the tests using it (while JDK 8 is working correctly).

@tacoo
Copy link
Contributor

tacoo commented Sep 5, 2017

Seems like oraclejdk7 is no longer supported.
So use openjdk7 or drop jdk7 completely?

travis-ci/travis-ci#7884 (comment)

@cowtowncoder
Copy link
Member

Excellent! Fixing travis builds now.

@cowtowncoder cowtowncoder reopened this Sep 7, 2017
@cowtowncoder
Copy link
Member

Ok: I am ready to commit this to master, since it's for 3.0.0 now, but looks like there's a conflicting change unfortunately. Would it be easy enough for you to resolve this?

I am hoping to do performance tests after merge: should not show any difference, but just want to verify. I also have some ideas for more code generation that I'd like to explore, giving me a good excuse to see how BB API works.

@geoand
Copy link
Contributor Author

geoand commented Oct 18, 2017 via email

@cowtowncoder
Copy link
Member

@geoand excellent, thank you in advance!

@geoand
Copy link
Contributor Author

geoand commented Oct 18, 2017 via email

@geoand
Copy link
Contributor Author

geoand commented Oct 22, 2017

@cowtowncoder While starting work on resolving the conflicts, I am failing to build various modules from source (for example jackson-core), because I can't find the source for jackson-base.
I am sorry if I am missing something obvious, but I tried searching the entire FasterXML organization and didn't come up with any Maven modules named jackson-base

Thanks in advance for your help!

@cowtowncoder
Copy link
Member

@geoand Apologies for confusion: jackson-base is bit hidden: it comes from jackson-bom, which just become multi-maven-project repo. Intent is for jackson-base to become parent pom for Jackson component themselves, and strip down jackson-bom to be something other projects may import or use as parent, but without getting too much of cruft that only Jackson projects require.

I thought I had deployed 3.0.0-SNAPSHOT of jackson-base to Sonatype snapshot repository, but perhaps this has not happened. Or maybe there's chicken and egg problem there.

Anyway, if you locally build jackson-bom, that should to the trick.

@geoand
Copy link
Contributor Author

geoand commented Oct 24, 2017 via email

@geoand geoand force-pushed the afterburner-bytebuddy branch from 6a4c4c4 to 1922e3c Compare October 28, 2017 09:20
@geoand
Copy link
Contributor Author

geoand commented Oct 28, 2017

Hi!

I just rebased the PR onto master.
Everything works correctly except one test, namely: TestUnwrapped.testSimpleSerialize.
However this test was not working on master branch either.

Update
As is evident, the build works on Travis, so the failing test must have been some issue with my local build

@cowtowncoder
Copy link
Member

@geoand Thanks: chances are failure may be due to changes in jackson-databind that maybe weren't synced (not sure if snapshot builds work right now), I'll verify things work for me, and if not see what is happening.

@cowtowncoder cowtowncoder merged commit 61685f4 into FasterXML:master Oct 30, 2017
@geoand
Copy link
Contributor Author

geoand commented Oct 31, 2017 via email

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.

4 participants