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

Optimize moshi decoding #2183

Merged
merged 4 commits into from
Sep 30, 2023
Merged

Conversation

iProdigy
Copy link
Contributor

@iProdigy iProdigy commented Sep 26, 2023

Follow-up to #2182

  • Decode JSON more efficiently by leveraging InputStream rather than Reader so the full response body does not need to be written to an intermediate String
  • Eagerly return null if the response body is empty (but not-null)

Summary by CodeRabbit

  • Refactor: Simplified and optimized the MoshiDecoder in the Moshi library. The changes include removing unused imports, replacing the Reader with BufferedSource from Okio for improved performance, and directly parsing JSON using jsonAdapter.fromJson(). This refactor enhances the efficiency of the decoding process and simplifies the underlying code structure.

@coderabbitai
Copy link

coderabbitai bot commented Sep 26, 2023

Walkthrough

The proposed changes streamline the Moshi decoder in Feign by eliminating unused imports and simplifying JSON parsing. The modifications enhance performance and readability, making the code more efficient and maintainable.

Changes

File Summary
.../feign/moshi/MoshiDecoder.java Unused imports removed. Replaced Reader with BufferedSource from Okio for improved performance. Simplified JSON parsing by directly using jsonAdapter.fromJson().

🎉🐇

In the land of code, where logic intertwines,

A rabbit hopped forth, seeking signs.

He nibbled at complexity, pruned it away,

Leaving a trail of simplicity, clear as day.

With each hop and skip, the code did lighten,

And the coder's burden, it did brighten. 🌟


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • Mention @coderabbitai in any review comment for bot assistance.
  • Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

  • Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between b5e6809 and 5507e52.
Files selected for processing (1)
  • moshi/src/main/java/feign/moshi/MoshiDecoder.java (3 hunks)
Files skipped from review due to trivial changes (1)
  • moshi/src/main/java/feign/moshi/MoshiDecoder.java

@@ -13,19 +13,16 @@
*/
package feign.moshi;

import com.google.common.io.CharStreams;
Copy link
Member

Choose a reason for hiding this comment

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

Can you also remove the guava dependency from pom?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, done in f3cd4d8

@iProdigy iProdigy requested a review from velo September 30, 2023 08:22
@velo velo merged commit c14c838 into OpenFeign:master Sep 30, 2023
1 check passed
@iProdigy iProdigy deleted the refactor/moshi-decode branch September 30, 2023 15:55
velo added a commit that referenced this pull request Oct 7, 2024
* perf: avoid writing full body to an intermediate string

* fix: eagerly return null for empty bodies

* chore(moshi): remove guava dependency

---------

Co-authored-by: Marvin Froeder <[email protected]>
velo added a commit that referenced this pull request Oct 8, 2024
* perf: avoid writing full body to an intermediate string

* fix: eagerly return null for empty bodies

* chore(moshi): remove guava dependency

---------

Co-authored-by: Marvin Froeder <[email protected]>
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