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

Allow use of public fields for getting/setting values #26

Closed
stephanenicolas opened this issue Jun 23, 2015 · 8 comments
Closed

Allow use of public fields for getting/setting values #26

stephanenicolas opened this issue Jun 23, 2015 · 8 comments
Milestone

Comments

@stephanenicolas
Copy link

If Jackson JR is designed for Android, it should be using fields. Getters and setters are useless and contribute to reach the dex limit of 65k methods. (Multidex is slow...)

If there was one thing to make mandatory, it would be to use public fields for pojos more than getters and setters.

I know you might be bugged by the problem to access private stuff. But you could also loosen that constraint, if you require the fields to be non-private, you can achieve a super fast lib. You could even create an annotation processor that generates parsing code, which would get read of any/most reflection. Reflection is also a problem on android that should be addressed by such a lib.

@cowtowncoder
Copy link
Member

I take this to mean "please allow using fields instead of or in addition to getters and setters", and will modify title appropriately. In future you may consider using titles that reflect feature you want to see, or defect to report.

Jackson jr is not specifically designed for Android, although it is one intended target.
For this reason, code generation is unlikely to be something I would add, as that would be Android specific. The concept of code generation is of course valid, and there are efforts to do that.
You might find this project:

https://github.com/Instagram/ig-json-parser/

interesting -- it also uses Jackson streaming parser/generator (like jackson jr), but then generates actual data-binding code.

@cowtowncoder cowtowncoder changed the title Getters and Setters is a bad choice Please allow access of Fields, not just Setter, Getter methods Jun 23, 2015
@stephanenicolas
Copy link
Author

Hi @cowtowncoder , thx for answering.

Sorry, the title was a bit dry and harsh. But still I maintain that IF there is one thing that should be mandatory (for now, using setters/getters), it should be the other way around and it should be fields that are mandatory. Allowing both fields and getters/setters would be nice, but I don't think they offer the same advantages.

On Android we need speed, and getters and setters are a lost of cpu cycles and moreover they increase dex methods count which is really a big Android constraint.

I don't see how code generation could be something to specific to Android. It is not. It would just allow to completely bypass reflection to find fields, the parsing code could be generated instead of having to build the parsing at runtime.

I have spent a lot of time studying Android's annotation processors, reflection, even byte code weaving...etc. And I am fully aware of alternatives like ig-gson-parser or logan Square.

I think you should really consider the option to force devs to use non private fields for pojos. That is actually the need of most advanced Android applications. If you want to give your lib a chance, I think that is the option. Again, for speed and dex method count reasons + the opportunity to generate parsing code completely.

Also, it might be interesting to have a look at okio/mochi to optimize usages of streams while parsing.

I am saying that as I love Jackson, but we are considering to move away from it soon as it doesn't offer the best performances on Android. Your project is really interesting to us but we won't use it if we have to increase the number of methods in our app and/or use reflection still.

@stephanenicolas stephanenicolas changed the title Please allow access of Fields, not just Setter, Getter methods Getters and Setters is a bad choice Jun 23, 2015
@stephanenicolas stephanenicolas changed the title Getters and Setters is a bad choice Getters and Setters Vs public fields Jun 23, 2015
@stephanenicolas stephanenicolas changed the title Getters and Setters Vs public fields Getters and Setters Vs non-private fields Jun 23, 2015
@cowtowncoder
Copy link
Member

Fair enough.

First of all I am not against adding functionality to access fields, if that can be done with relatively modest additions. That actually sounds potentially doable. And that's also how Jackson originally started (Fields were added in 1.1).So if you would be interested in prototyping something for jackson-jr, I would be interested in helping you.

Going backwards from that, jackson-jr specifically, one big goal is to keep package small and this is the main reason for leaving out much of what full Jackson support, including fields.
But that does not mean that it wouldn't be possible to add field access, or that I thought it would not be possible addition. I just want things to be kept tight all around. Reflection will still be needed; I don't know how much speed difference Field vs Method access makes: on standard J2ME, after forcing access, I haven't seen much difference, but it sounds like there is bigger difference on Android?

As to code generation, Android bytecode is different from Java bytecode; and so my preferred method of on-the-fly generation differs between JVM and Dalvik.
This is the reason why jackson-module-afterburner does not work on Android; it would actually be great to create Dex equivalent of it. And I would be interested in such an approach, but unfortunately have not worked on Dex or libraries; nor know whether dynamic loading actually works. Perhaps it would require build-time processing, which to me is much less interesting approach.
The alternative of source-code generation is technically viable, but I am personally not interested in that approach, either on JVM or Dalvik. Others find it interesting and are working on that; choice is good.

@stephanenicolas
Copy link
Author

@cowtowncoder

You didn't fully understand my point. Sorry, I was not very clear :
There is no difference on Android between using reflection for accessing fields or accessing methods. The point is that the whole reflection is slow on Android. Any reflection call is costly, that's why people try to pre-compute as much as possible with annotation processing.

The main advantage of using fields for Android is immediate : it decreases the dex methods count. Android is limited to 65k methods per app, multidex allows to bypass this limit but there is an overhead and it's slower than not using it.


Other topic : code generation vs reflection. Whether you decide to keep setters/getters or fields, you could use an annotation processor to completely bypass using reflection at runtime: you could generate the code for parsing at compile time.

It looks like you are talking about generating byte code (that would be specific to a JVM), but that is not what I am talking about. The traditional approach on Android is to generate Java source code, not byte code, and this is fully portable on any JVM. I understand it's a tough path, but from a performance point of view, and also for build compatibility issues, that's the only and best solution around for Android.

The point is that without code generation, and with reflection, chances are that all efforts you are making to get a new library working well on Android are lost. There are already a few alternatives around that will perform better, sorry about that. Reflection is really a BIG issue on Android's performances.


What about okio ?


If you/we (as we are pretty interested to an optimal solution based on Jackson) could achieve to combine :

  • Jackson API / annotations compatibility
  • public fields usage to decrease dex methods count
  • code generation instead of reflection
  • maybe use okio to enhance performances of streams

--> Your lib would be great and a few steps ahead of what is currently available today. There is a tiny niche for it. Otherwise I don't think the lib has a real chance of success on Android.

@cowtowncoder
Copy link
Member

Ok. So sounds like there are 2 parts:

  1. Use of public fields (or perhaps others, but that's a minor thing): use would have benefits wrt code size. So it sounds like a low-hanging fruit, with modest benefits.
  2. Source code generation, using Jackson as the source for model, with annotations etc.

I think that at very high level, (2) makes sense. There are 2 projects in fasterxml space that do something related, I think (aside from Afterburner):

Latter could serve as sort of basis; and actually couple of other dataformat modules also use this: CSV, Avro and Protobuf modules all generate internal schema representations to adapt native external format into more JSON like events.
I have also had some ideas regarding writing lib like "BeanMate", that would actually expose better refined POJO model representation, for general-purpose POJO handling; something that works at higher level than ClassMate (https://github.com/FasterXML/java-classmate), using it for type resolution, but then combining logical properties, with naming overrides, typing and such.
And exposing those using abstractions that do not rely on Jackson types, but rather allow alternate annotation and config sources.

At more practical level, it is an open question as to whether this could fit with jackson-jr as a sub-module. I do not think it fits within core itself, for multiple reasons, but as an add-on, perhaps.
More likely it would make sense as a new project (jackson-android?), with explicit goal of source generation, using jackson-databind as the source of object models to use.

Having written above, I think the new project idea would make most sense, if anyone wants to pursue that. I do not have time or immediate need to pursue it, but I wouldn't be surprised if others within Jackson community would be very interested. Your points are valid, regarding Android usage, challenges; beyond challenges of using reflection for access, performance of annotation reading is apparently abysmal. I guess engineers didn't think anyone would use them on runtime or something.

You could suggest such a project on Jackson discussion groups (jackson-dev at Google groups) to see how others feel.

@stephanenicolas
Copy link
Author

Thx for the reply again @cowtowncoder : just a point about your 1) it's really a big thing on Android to decrease dex methods count. Some libs can add up to 30k methods, most of them thousands. This is a real issue on Android. It's really NOT about having to type getters and setters, not at all, but about having an app that fits into a single dex file.

Every method you save is good to be saved, and here we are talking about thousands of methods in large app.

For a different project specific to Android, well if that happens we would be glad to contribute. Now. But my guess is that's it's going to happen outside of the jackson community sooner.

@cowtowncoder
Copy link
Member

Sounds reasonable. I'll keep this as a feature request for (1). The other part is up to whoever has the itch then.

@cowtowncoder cowtowncoder changed the title Getters and Setters Vs non-private fields Allow use of public fields for getting/setting values Jun 16, 2016
@cowtowncoder cowtowncoder added this to the 2.8.0 milestone Jun 16, 2016
@cowtowncoder
Copy link
Member

Implemented; feature must be enabled by:

JSON j = JSON.std.with(Feature.USE_FIELDS);

and with that public fields now infer existence of property with same name.

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

No branches or pull requests

2 participants