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

Avoid use of reflection-based deserialization in Java model #194

Merged
merged 4 commits into from
Mar 20, 2019

Conversation

RicoYao
Copy link
Contributor

@RicoYao RicoYao commented Mar 20, 2019

A reflection-based adapter is not as performant. Since Plank is aware of the type of every field, it can generate a TypeAdapter for each type and route each json element in the tree to the appropriate one.

#193

Also switched to using the Builder to generate the model, so that we don't have to manually hack the _bits field. We may have to revert that if we find there's a performance hit to calling setters+build instead of just setting variables directly.

@rahul-malik
Copy link
Collaborator

@RicoYao Nice! For the setter concern it might be worth seeing what ProGuard does since it might perform that optimization for you?

@RicoYao
Copy link
Contributor Author

RicoYao commented Mar 20, 2019

@RicoYao Nice! For the setter concern it might be worth seeing what ProGuard does since it might perform that optimization for you?

@rahul-malik will do! Also, sorry, I pushed one more commit to the PR to avoid the unnecessary usage of TypeTokens for basic types.

@@ -213,6 +213,15 @@ public struct JavaModelRenderer: JavaFileRenderer {

// MARK: - TypeAdapter

func typeAdapterVariableNameForType(_ type: String) -> String {
return (type
.replacingOccurrences(of: "<", with: "_")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we refactor this logic as an extension on String? I think we do this in a couple of places throughout the codebase to convert a type to a valid variable name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added replacingNonAlphaNumericsWith

@rahul-malik
Copy link
Collaborator

@RicoYao - No worries, looks good. I'll merge when CI passes

@rahul-malik rahul-malik merged commit d467436 into pinterest:master Mar 20, 2019
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.

2 participants