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

Cannot get next value -- unkeyed container is at end #24

Closed
zabolotiny opened this issue Mar 11, 2020 · 38 comments · Fixed by #33
Closed

Cannot get next value -- unkeyed container is at end #24

zabolotiny opened this issue Mar 11, 2020 · 38 comments · Fixed by #33

Comments

@zabolotiny
Copy link

I am trying to extend Lottie-Ios with Zippy Json.
But when i use standard init: try ZippyJSONDecoder().decode(Animation.self, from: json)
It throws exception:

DecodingError
▿ valueNotFound : 2 elements
- .0 : Any
▿ .1 : Context
▿ codingPath : 2 elements
▿ 0 : JSONKey(stringValue: "layers", intValue: nil)
- stringValue : "layers"
- intValue : nil
▿ 1 : JSONKey(stringValue: "Index 1", intValue: 1)
- stringValue : "Index 1"
▿ intValue : Optional
- some : 1
- debugDescription : "Cannot get next value -- unkeyed container is at end."
- underlyingError : nil

JSON Attached
SpinnerLoader.json.zip

@zabolotiny
Copy link
Author

@michaeleisel Can you please give me a hint how to fix this?

@zabolotiny
Copy link
Author

zabolotiny commented Mar 12, 2020

@michaeleisel The issue happens here:
func decode<T>(_ type: T.Type) throws -> T where T : Decodable { try ensureArrayIsNotAtEnd() let decoded = try decoder.unbox(currentValue, as: T.self) advanceArray() return decoded }
image

it is called from extension for decoding a heterogeneous list of objects for a given family in Lottie library

func decode<T : Decodable, U : ClassFamily>(_ heterogeneousType: [T].Type, ofFamily family: U.Type, forKey key: K) throws -> [T] { var container = try self.nestedUnkeyedContainer(forKey: key) var list = [T]() var tmpContainer = container while !container.isAtEnd { let typeContainer = try container.nestedContainer(keyedBy: Discriminator.self) let family: U = try typeContainer.decode(U.self, forKey: U.discriminator) if let type = family.getType() as? T.Type { list.append(try tmpContainer.decode(type)) } } return list }

image

Maybe you can give some hint here?

@michaeleisel
Copy link
Owner

I'll take a look soon

@michaeleisel
Copy link
Owner

Could you send me a main.swift that reproduces the issue?

@zabolotiny
Copy link
Author

@michaeleisel Hi, can you please check the fork of lottie https://github.com/sheff1422/lottie-ios.git . Please use branch ZippyJSON . Just run example on the simulator and you will see the issue.
I will be checking this thread during today so let me know if i can help u somehow.

@michaeleisel
Copy link
Owner

The issue is with having multiple simultaneous references to an unkeyed container, and how mutations to one affect the other with ZippyJSON. Swift makes things difficult without a copy constructor

@zabolotiny
Copy link
Author

@michaeleisel I also came up to this yesterday. But not sure what solution i can apply safely without breaking anything.... Any ideas?

@michaeleisel
Copy link
Owner

Let me think about how to solve it, it may require some architectural changes

@zabolotiny
Copy link
Author

@michaeleisel I am ready to participate. Maybe you can share your ideas?

@michaeleisel
Copy link
Owner

it may involve memcpy to replicate C++ objects, which is technically undefined behavior. it will need some experimentation, and i don't want to have you do changes only to reject them. i'll take care of this in the next couple weeks

@zabolotiny
Copy link
Author

@michaeleisel Sorry for bothering u. How it is going? I am looking for some alternative ways to fix performance at our project rn but your library looks very promising.

@michaeleisel
Copy link
Owner

Hi, it may take a bit for me to do this. You’re free to fork either lottie or my project to do what you want

@michaeleisel
Copy link
Owner

I haven't forgotten about this, a fix is incoming. Just a lot of yak shaving to do for it

@michaeleisel
Copy link
Owner

if you have time, please give latest master a shot and see if it fixes it, i.e. as part of release testing before i cut a release

@michaeleisel
Copy link
Owner

(also, make sure you fix ZippyJSONCFamily, its dependency, at that one's latest master), e.g. using dev cocoapds

@zabolotiny
Copy link
Author

@michaeleisel I will check that during nearest 2 days

@zabolotiny
Copy link
Author

@michaeleisel Unfortunately the problem is still there :(
valueNotFound(Any, Swift.DecodingError.Context(codingPath: [JSONKey(stringValue: "layers", intValue: nil), JSONKey(stringValue: "Index 9", intValue: 9)], debugDescription: "Cannot get next value -- unkeyed container is at end.", underlyingError: nil))
To reproduce you can check this repo:
https://github.com/sheff1422/lottie-ios.git
branch: Zippy-Json-experiments

@zabolotiny
Copy link
Author

@michaeleisel 👆

@michaeleisel
Copy link
Owner

yeah i see it

@michaeleisel
Copy link
Owner

new fix is in latest master, i've tested your case and it goes from broken to fixed

@zabolotiny
Copy link
Author

@michaeleisel thank you, i will test tomorrow in the morning and get back to this ticket with the feedback

@zabolotiny
Copy link
Author

@michaeleisel Another update is required to make it work
image
After this change all looks correct.
I will measure performance tonight to understand the difference

@zabolotiny
Copy link
Author

@michaeleisel After that change it works but has a huge performance issues.
Decode times:

  1. try ZippyJSONDecoder().decode
    Time to decode animation 153.78289997577667 sec

  2. try JSONDecoder().decode
    Time to decode animation 1.1195780038833618

Tested on iPhone 7.
Maybe you can recommend something?

@zabolotiny
Copy link
Author

zabolotiny commented Jun 28, 2020

@michaeleisel Latest ver is available here https://github.com/zabolotiny/lottie-ios branch: Zippy-Json-experiments

@michaeleisel
Copy link
Owner

what's your code to test for equality? JSONEncoder sadly produces non-deterministic output

@michaeleisel
Copy link
Owner

by the way, if you'd like to add a some lottie decoding tests to the test suite, it'd help a lot. i can provide any support if you're interested

@michaeleisel
Copy link
Owner

investigating further, i see what you mean about the bug. i will fix that. as for the performance, it's tricky because the slowdown comes from all the exceptions being thrown during json parsing, where it computes the codingPath. zippy json is optimized to only compute this in exceptional circumstances, and so here it's slower than apple

@michaeleisel
Copy link
Owner

please try 1.2.1 and see if it works better for you - it looks good for me

@zabolotiny
Copy link
Author

@michaeleisel Sorry for late reply. I will check that. We are launching on the new market so i had no chance to check notifications. I will try get back to you with some info during this week.
Just some feedback from 1.1 -> when i was trying to change the build config and use production one. Time comparison was:
JSONDecoder().decode -> 1sec
ZippyJSONDecoder().decode -> 5sec

@michaeleisel
Copy link
Owner

Yes, there was a clear bottleneck that has now been fixed

@zabolotiny
Copy link
Author

@michaeleisel Sorry for late response, I was testing it and with my complicated animation pack again:
ZippyJSON 1.2.1
iPad Air 1
Lottie (JSONDecoder) -> 3.3701289892196655s
Lottie (ZippyJSON) -> 3.8821139335632324s

@zabolotiny
Copy link
Author

@michaeleisel One more thing. When I say 1.2.1 -> I mean latest master.
If you think there is a chance to significantly improve performance I would love to help with some Lottie decoding tests.

@michaeleisel
Copy link
Owner

Hi, can you explain exactly how to run this benchmark and reproduce these results? Also, have you ensured that you're testing with Release mode and not Debug?

@michaeleisel
Copy link
Owner

Also, I noticed in Zippy-Json-experiments that it's not on 1.2.1, it's on 1.2.0

@zabolotiny
Copy link
Author

zabolotiny commented Dec 16, 2020

@michaeleisel Just noticed your message, I will try to find some time on weekends to setup easy project for you.
P.S. the version with 1.2.1 was not pushed. And I used debug mode, I will test in release also and let you know.
P.P.S.: It's pre Christmas rush here because of Appstore review team holidays. We want to ship a lot of stuff. So I am sorry for late replies.

@michaeleisel
Copy link
Owner

Sounds good. And what do you mean "not pushed"? I see it in https://github.com/michaeleisel/ZippyJSON/tags and https://cocoapods.org/pods/ZippyJSON (but sometimes Cocoapods is bad at updating these things)

@zabolotiny
Copy link
Author

@michaeleisel Finally I am back again after releasing a big feature on my main project.
Fresh experiment is showing a great boost.
ZippyJSON 1.2.1
iPhone 7
Lottie (JSONDecoder) -> 0.326s
Lottie (ZippyJSON) -> 0.08 - 0.176s
Values significantly change when you test in release mode.
I want to add this to my project so I will make a PR to Lottie branch also in the nearest time.

@michaeleisel
Copy link
Owner

Nice!

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 a pull request may close this issue.

2 participants