ion-rust icon indicating copy to clipboard operation
ion-rust copied to clipboard

Re-evaluate `TokenStream` Implementation Strategy

Open almann opened this issue 2 years ago • 0 comments

Regarding #540/#541/#542, with #546 in flight and Ion 1.1 macro development on-going, the TokenStream API is useful, but its implementation is not efficient due to a number of reasons. Once the dust settles, we should re-evaluate how to implement TokenStream once we have better interfaces for IonReader and friends. I think this will necessarily mean refactoring these APIs, but I am hesitant to do it while we are still working through these other APIs.

There's a lot of machinery here in support of the field/annotations/content Thunk tuple. Given that this model exists to accommodate the limitations of the current IonReader trait (which we hope to improve soon), I wonder if it wouldn't be better in the near term to materialize each value to keep the code simpler. As it is, each Thunk is a Box<dyn...>, so I think we're doing 3 heap allocations and dynamic dispatch for every token even when we don't read those fields, and up to 6 heap allocations when we do. Materializing things as we encounter them would just be 3 for each token.

Originally posted by @zslayton in https://github.com/amazon-ion/ion-rust/pull/540#pullrequestreview-1415197400

almann avatar May 13 '23 18:05 almann