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

Use moshi implementation for json parsing #1234

Merged
merged 19 commits into from
May 30, 2019
Merged

Use moshi implementation for json parsing #1234

merged 19 commits into from
May 30, 2019

Conversation

gpeal
Copy link
Collaborator

@gpeal gpeal commented May 27, 2019

There's a dependency on OKIO & 8 classes copied from Moshi. Need to figure out best path forward. Ideally we don't depend on all of Moshi but still get the gains of the new JsonReader & Options api. This should fix the random Android 8 crashes.

Performance

I ran the snapshot tests with ~1800 animations and summed up just the parsing time. The old parsing code took 13,145ms and 13,645ms on each test run (avg 13,395ms). The new code took 12,858ms each time. There aren't enough trials to deduce statistical differences but if these numbers hold, the new code parses ~5% faster.
It does reduce memory allocations during parsing which may contribute to the performance improvement.

I'm leaning on merging this to hopefully fix #667

fyi: @digitalbuddha

@gpeal
Copy link
Collaborator Author

gpeal commented May 27, 2019

Snapshots Failed

@gpeal
Copy link
Collaborator Author

gpeal commented May 27, 2019

Snapshots Failed

@gpeal
Copy link
Collaborator Author

gpeal commented May 27, 2019

Snapshot Tests
28: Report Diff

@gpeal gpeal changed the title [WIP] okio and moshi json parsing Okio and moshi json parsing May 27, 2019
@gpeal gpeal changed the title Okio and moshi json parsing Use moshi implementation for json parsing May 27, 2019
@gpeal
Copy link
Collaborator Author

gpeal commented May 27, 2019

Snapshot Tests
28: Report Diff

1 similar comment
@gpeal
Copy link
Collaborator Author

gpeal commented May 27, 2019

Snapshot Tests
28: Report Diff

@gpeal gpeal merged commit 2354bad into master May 30, 2019
@gpeal gpeal deleted the mike/moshiJsonReader branch May 30, 2019 20:57
@OussamaHaff
Copy link

Can we have this bug fix on a non AndroidX version of Lottie please (ex 2.7.x)

@gpeal
Copy link
Collaborator Author

gpeal commented Jun 26, 2019

@hfrsoussama I made a 3.0.3-support version but it takes a lot of manual work and I'm not sure I'm going to do it again. Maybe I'll do one more but if you want to make a branch from 3.0.7 and revert the androidx migration and make a PR, I can publish it.

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.

JSON Reader AssertionError when doInBackground (Android 7, 8)
4 participants