Closed NobodyXu closed 1 year ago
Thanks! I find it strange that it's possible to declare something Sync
while it has methods that require &mut self
.
Something I'd like to do here is to drive the changes by required refactoring in gix-*
crates just to verify they can work in practice. Because once subprogress starts to happen, things get even more complicated and… let's just say I have been burned enough without trying to make it dyn
-safe 😅.
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 gix-*
can serve as example there as well.
Something I'd like to do here is to drive the changes by required refactoring in
gix-*
crates just to verify they can work in practice. Because once subprogress starts to happen, things get even more complicated and… let's just say I have been burned enough without trying to make itdyn
-safe 😅.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
gix-*
can serve as example there as well.
I implemented Progress
for BoxedDynProgress
and implemented DynProgressToProgressBridge
which also implements Progress
for any type that implements DynProgress
.
I had to adjust BoxedDynProgress
to take 'static
type as a result of that, and I think this actually "test" the typing system I created here to be actually usable.
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 BoxedDynProgress
and pass it to other tests.
I had to adjust BoxedDynProgress to take 'static type as a result of that, and I think this actually "test" the typing system I created here to be actually usable.
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 Progress
trait itself should be done first, as this should simplify follow-up work with providing optional dyn-safety.
More radically, I wonder if SubProgress
is even needed anymore because this should always be the same type as the parent progress - from my experience it only has been complicating things.
Lastly, once Progress
becomes Sync
due to &self
, I think it can also be Clone
which wasn't done by design previously, but once again, I think reality shows that it would be easier to use if it was Clone
and the defacto standard implementation progress::tree
can satisfy all of these constraints already.
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 gix
.
What do you think?
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.
Sure, I can add testing for DynProgress.
More radically, I wonder if
SubProgress
is even needed anymore because this should always be the same type as the parent progress - from my experience it only has been complicating things.
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?
which wasn't done by design previously, but once again, I think reality shows that it would be easier to use if it was
Clone
and the defacto standard implementationprogress::tree
can satisfy all of these constraints already.
Hmmm, in this case, maybe we can require the implementation to be wrapped in Arc and support a DynClone trait which returns Arc
Sure, I can add testing for DynProgress.
I think it would be better to hold off with this and refactor Progress
first - from there as you said, it might be dyn
safe naturally.
Is it possible to avoid returning sub-progress entirely?
I think it should be, but there is only one way to tell. gix
was the driver for the current complexity, but type SubProgress
was there from the very beginning and I am not sure was made me add it in the first place… oh wait… there are these wrapper types like DoOrDiscard
which are able to wrap a Progress
impl via generics. In order to add_child()
on those one would need… no wait, it should always be possible to define it as add_child() -> Self
essentially. But please try it with this.
From there everything should fall into place neatly :).
Hmmm, in this case, maybe we can require the implementation to be wrapped in Arc and support a DynClone trait which returns Arc?
Sorry for bringing it up if it complicates things - we can ignore it for now as Clone
is absolutely optional, gix
doesn't use it as it didn't exist before.
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 :)
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 Progress
could remain monomorphic also in gix-*
plumbing crates and the caller has to make sure only one implementation of Progress
is used. For gix
(CLI) this is already the case, so no duplication should stem from this. That way the proposed means of obtaining progress remains the fastest possible. With that dyn-progress wouldn't be all that useful, even though I wonder if there is also other ways to get it to be dyn-safe. I probably will give it a shot.
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'.
It seems that set, inc and getting steps are extremely simple for dyn overhead to be that large. Would it be reasonable to directly return reference to underlying step instead?
E.g.
fn step(&self) -> &AtomicUsize;
Something like it exists already and is used as well. Less for performance, more for convenience (counter()
).
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 cargo-bloat
that there is no tool to help with that, yet, so best-practices should be followed across the codebase soon. This is something I noticed in the cargo
codebase by the way, so I am sure they went through similar motions.
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 Store
effectively, which is much faster than fetch_add
- the latter now makes 500Melem/s, and behind dyn it's styll 450Melem/s, so it's not really an issue for this kind of workload at all. Thus Progress
should really be dyn
everywhere in plumbing.
Fixed #21
DynProgress
is sealed and implemented for any type that implementsProgress
.