-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat: Add new trait DynProgress
& type BoxedDynProgress
#22
Conversation
Thanks! I find it strange that it's possible to declare something Is this something you could do? If not, how do you validate it? I really think this needs tests to simulate how it would be used, maybe |
Fixed Byron#21 `DynProgress` is sealed and implemented for any type that implements `Progress`. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
44c3773
to
24d0b2a
Compare
I implemented I had to adjust All the other code is just forwarding result from one to another, so IMHO it doesn't need to be tested. If testing is required, we could construct a |
I see, thanks for the clarification. It's an implicit test and I think despite that it would be useful to have explicit ones - there should be examples in the codebase already. Something I find makes this PR more difficult to deal with is me thinking that the adjustments to the Lastly, once If you see this similarly, please feel free to use this PR to make all modifications - my goal would be to eventually test it against an actual function in What do you think? |
Sure, I can add testing for DynProgress.
If we can remove SubProgress, remove all the impl Into trait used in the trait, then we could try making Progress dyn-safe instead. Is it possible to avoid returning sub-progress entirely?
Hmmm, in this case, maybe we can require the implementation to be wrapped in Arc and support a DynClone trait which returns Arc? |
I think it would be better to hold off with this and refactor
I think it should be, but there is only one way to tell. From there everything should fall into place neatly :).
Sorry for bringing it up if it complicates things - we can ignore it for now as And again, apologies for all these changes, this is a complex task and one that is easily underestimated, hard to predict. If you don't feel to take it on, I definitely offer to give it a shot myself in a timely manner. |
I can try implementing the idea you suggested as a separate PR, so if it fails we can always fallback to this one. |
Sounds like a plan! Thanks for your help :) |
They are roughly half as fast due to the dynamic call overhead.
I looked at this PR again and came up with some benchmark. According to these, the dyn-call overhead cuts the performance in half for setting/incrementing the progress. This is expected, I just found it interesting as I had no idea previously beyond 'dyn calls are slow'. With this knowledge, I feel that ideally |
It seems that set, inc and getting steps are extremely simple for dyn overhead to be that large. E.g.
|
Something like it exists already and is used as well. Less for performance, more for convenience ( Yesterday I noticed that the main concern should be towards closures/callbacks, as these most often will be causing duplication even if the closure effectively does the same. I feel a strong hunch to review the codebase and find a solution for these now rather than keep going with it, and see it duplicate out of control. Particularly now that I learned from |
Thanks again for this contribution! I was definitely inspired by it to rethink progress once again and simplify parts of it. And of course, now nested progress can be dynamic and I will see where it should be applied. I also realized that the performance benchmarks were misleading as they tested |
Fixed #21
DynProgress
is sealed and implemented for any type that implementsProgress
.