Call `flushCallBuffer` at regular intervals #192

massudaw · 2017-06-17T01:13:51Z

This is a workaround for batches needing explicit calls to flushBuffersCall

meteficha · 2017-08-25T12:11:50Z

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.

massudaw · 2017-08-25T12:30:31Z

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.

meteficha · 2017-08-25T12:50:05Z

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.

massudaw · 2017-08-25T12:57:22Z

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.

meteficha · 2017-08-25T15:45:11Z

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.

HeinrichApfelmus · 2017-08-25T16:09:53Z

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.

massudaw · 2017-08-25T18:36:28Z

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.

This mode is like `BufferRun`, but will flush the call buffer every 300ms if nonempty.

HeinrichApfelmus · 2017-08-27T20:12:13Z

I have implemented a new call buffer mode, FlushPeriodically, in commit cdc8f40 , which essentially implements the pull request here. @massudaw , does this work for you?

This mode is like `BufferRun`, but will flush the call buffer every 300ms if nonempty.

massudaw · 2017-08-28T02:17:20Z

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

HeinrichApfelmus · 2017-08-28T16:40:51Z

src/Foreign/JavaScript/CallBuffer.hs

@@ -5,6 +5,8 @@ import Control.Concurrent
 import Control.Concurrent.STM as STM
 import Control.Monad

+import GHC.Conc


Importing a GHC-specific module is not a good idea. I would prefer to use functions from standard modules like Control.Concurrent only.

HeinrichApfelmus · 2017-08-28T16:42:30Z

src/Foreign/JavaScript/EventLoop.hs

@@ -23,6 +23,8 @@ import qualified System.Mem
 import Foreign.RemotePtr             as Foreign
 import Foreign.JavaScript.CallBuffer
 import Foreign.JavaScript.Types
+import Data.Time
+import GHC.Conc


HeinrichApfelmus · 2017-08-28T16:48:36Z

threepenny-gui.cabal

@@ -119,6 +119,7 @@ Library
                    ,file-embed             == 0.0.10
                    ,hashable               >= 1.1.0 && < 1.3
                    ,safe                   == 0.3.*
+                    ,time                   >= 1.6.1 && < 1.9


I would like to have time >= 1.4.* here, to ensure compatibility with older GHC versions.
We have CI in place to test whether they compile.

HeinrichApfelmus · 2017-08-28T16:49:39Z

src/Foreign/JavaScript/Types.hs

@@ -251,10 +252,11 @@ data CallBufferMode
    -- to simplify usage. Users may choose 'BufferRun' instead if they want more control
    -- over flushing the buffer.
    | FlushPeriodically
+      { max_buffer_timeout :: Int


The standard style here is CamlCase, I would prefer this to be maxBufferTimeout.

HeinrichApfelmus · 2017-08-28T16:52:30Z

src/Foreign/JavaScript/EventLoop.hs

+flushCallBufferPeriodically w@Window{..} = forever $ do
+  b <- atomically $ do
+    tl <-  takeTMVar wCallBufferStats
+    FlushPeriodically max_flush_delay <- readTVar wCallBufferMode


I would prefer CamlCase, maxFlushDelay.

HeinrichApfelmus · 2017-08-28T17:00:01Z

src/Foreign/JavaScript/EventLoop.hs

+        flushCallBuffer w
+        threadDelay (delta*1000)
+    Left delta ->
+        threadDelay (delta*1000)


Move threadDelay out of the case expression, eliminating one copy.

HeinrichApfelmus · 2017-08-28T17:01:53Z

src/Foreign/JavaScript/Types.hs

@@ -265,6 +267,7 @@ data Window = Window

    , wCallBuffer     :: TVar (String -> String)
    , wCallBufferMode :: TVar CallBufferMode
+    , wCallBufferStats :: TMVar UTCTime


I think the code can be clarified by using an ordinary TVar instead.
The single use of takeMVar can be rewritten by using the retry function.

HeinrichApfelmus · 2017-08-28T17:04:36Z

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.

massudaw mentioned this pull request Jun 17, 2017

onEvent does not always update display #191

Closed

HeinrichApfelmus added the Status: In Progress label Aug 13, 2017

HeinrichApfelmus mentioned this pull request Aug 27, 2017

DrumMachine question #199

Closed

HeinrichApfelmus changed the title ~~Force flush of buffer~~ Call flushCallBuffer at regular intervals Aug 27, 2017

HeinrichApfelmus added a commit that referenced this pull request Aug 27, 2017

Add new CallBufferMode: FlushPeriodically. #192

cdc8f40

This mode is like `BufferRun`, but will flush the call buffer every 300ms if nonempty.

HeinrichApfelmus added Type: Design Discussion Type: Enhancement and removed Status: In Progress labels Aug 27, 2017

massudaw pushed a commit to massudaw/threepenny-gui that referenced this pull request Aug 28, 2017

Add new CallBufferMode: FlushPeriodically. HeinrichApfelmus#192

1751a45

This mode is like `BufferRun`, but will flush the call buffer every 300ms if nonempty.

massudaw force-pushed the flush-buffer branch from 9f79612 to 31bd6c1 Compare August 28, 2017 02:04

improve period flushing

04a064f

massudaw force-pushed the flush-buffer branch from 31bd6c1 to 04a064f Compare August 28, 2017 02:07

HeinrichApfelmus requested changes Aug 28, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call `flushCallBuffer` at regular intervals #192

Call `flushCallBuffer` at regular intervals #192

massudaw commented Jun 17, 2017

meteficha commented Aug 25, 2017

massudaw commented Aug 25, 2017

meteficha commented Aug 25, 2017

massudaw commented Aug 25, 2017 •

edited

Loading

meteficha commented Aug 25, 2017

HeinrichApfelmus commented Aug 25, 2017

massudaw commented Aug 25, 2017

HeinrichApfelmus commented Aug 27, 2017

massudaw commented Aug 28, 2017

HeinrichApfelmus Aug 28, 2017

HeinrichApfelmus Aug 28, 2017

HeinrichApfelmus Aug 28, 2017

HeinrichApfelmus Aug 28, 2017

HeinrichApfelmus Aug 28, 2017

HeinrichApfelmus Aug 28, 2017

HeinrichApfelmus Aug 28, 2017

HeinrichApfelmus commented Aug 28, 2017

Call flushCallBuffer at regular intervals #192

Are you sure you want to change the base?

Call flushCallBuffer at regular intervals #192

Conversation

massudaw commented Jun 17, 2017

meteficha commented Aug 25, 2017

massudaw commented Aug 25, 2017

meteficha commented Aug 25, 2017

massudaw commented Aug 25, 2017 • edited Loading

meteficha commented Aug 25, 2017

HeinrichApfelmus commented Aug 25, 2017

massudaw commented Aug 25, 2017

HeinrichApfelmus commented Aug 27, 2017

massudaw commented Aug 28, 2017

HeinrichApfelmus Aug 28, 2017

Choose a reason for hiding this comment

HeinrichApfelmus Aug 28, 2017

Choose a reason for hiding this comment

HeinrichApfelmus Aug 28, 2017

Choose a reason for hiding this comment

HeinrichApfelmus Aug 28, 2017

Choose a reason for hiding this comment

HeinrichApfelmus Aug 28, 2017

Choose a reason for hiding this comment

HeinrichApfelmus Aug 28, 2017

Choose a reason for hiding this comment

HeinrichApfelmus Aug 28, 2017

Choose a reason for hiding this comment

HeinrichApfelmus commented Aug 28, 2017

Call `flushCallBuffer` at regular intervals #192

Call `flushCallBuffer` at regular intervals #192

massudaw commented Aug 25, 2017 •

edited

Loading