Open massudaw opened 7 years ago
This seems like a bad idea to me. I'd rather have two flushes that occur at the right time, than a single nice flush that appears with a delay because it got caught by the timer.
Also, although unlikely, this timer could split an otherwise good flush into two.
This is really a trivial fix. But the current flush mechanism is also quite ad-hoc and causes problems with new users. Also i find quite unnecessary explicit flushing.
What do you describe as a good flush? I've been using this for more the a year and never seem any breakage, because of split.
I think a maximum time bound from the last flush and probably a maximum bound on request size is desired. So we can keep user delay and request size bounded.
I was saying that it's better to have an explicit flush after every onEvent
rather than relying on the timer. So a good flush would be one that is as big as possible.
The timer surely works, but it adds a delay to the UI, which is undesirable.
But this doesn't disable the explicit flush. You can still flush to minimize delay.
When you have more then one onEvent and all commands inside those can be batched you start creating unnecessary flushes and doesn't get "good flush" if you use a flush on every onEvent
.
I guess what I'm trying to say is that right now it won't work at all if you don't flush at the correct places. That makes it obvious (if know about flushing, of course) that something is wrong.
With this PR, though, it's always going to work. However, if you miss a flush at the right place, your GUI is going to have more latency. That's subtle and hard to track.
So I don't like this PR because it turns an obvious problem into something that works but lags.
It could become an additional CallBufferMode
, though, for people who like it. I think it's at least a valid way to deal with buffering. We may want to look into old documentation for screen terminals, they must have dealt with similar issues back in the day.
We can improve latency without compromising batch size by using a timeout from the last written command. Instead of polling periodically.
Or maybe we have some explicit way to know when the event graph is finished or blocked and flush.
I don't really like this fixed interval also. It's just the most trivial fix for the problem that i could think.
I have implemented a new call buffer mode, FlushPeriodically
, in commit cdc8f406bf5f9ca114840b4deaeddf3004cb0517 , which essentially implements the pull request here. @massudaw , does this work for you?
Yep seem to solve my problem. I've implemented an improved version that reduces latency, avoids splitting a buffer when it's being written and doesn't. If you think is worth adding the complexity
I've implemented an improved version that reduces latency
Nice, thanks! There are a couple of changes that I would like to see, if you are up to it. Otherwise, I'm happy to leave this issue open for now and return to it at a later point.
This is a workaround for batches needing explicit calls to flushBuffersCall