Fix infinite memory usage by allowing old data to be garbage collected #2473
Conversation
alerque
added
the
A-BUG
|
See issue comments for updates on testing, I did do a longer term run test and got more conclusive results than the initial speculation here. |
|
I haven't had much quality hacking time just lately - but I am loving this burst of activity, will follow up asap! |
|
I just converted this to a draft again because the latest commits here have not been properly vetted. I'm hacking wildly trying to make out where the leak(s) are. The current iteration runs much better for me including not leaking memory from the registry screen, but I haven't reasoned through everything enough to verify functionality won't have been lost. |
| e | e `elem` [AppEvent FileChange, VtyEvent (EvKey (KChar 'g') [])] -> uiReload copts d ui >>= put' | ||
| e | e `elem` [AppEvent FileChange, VtyEvent (EvKey (KChar 'g') [])] -> do | ||
| ui' <- uiReload copts d ui | ||
| put' $ regenerateScreens (ajournal ui') d ui' |
There was a problem hiding this comment.
Reloading in the register screen now has an extra call to regenerateScreens. Does this help ? I think it's also being called by uiReload. Needs an explanatory comment at least.
| -- sendVtyEvents [EvKey KEnter [], EvKey (KChar 'g') []] -- XXX Might be disrupted if other events are queued | ||
| tsReload copts d ui = uiReload copts d ui >>= put' | ||
|
|
||
| tsReloadIfFileChanged copts d j ui = liftIO (uiReloadIfFileChanged copts d j ui) >>= put' |
There was a problem hiding this comment.
This special handling for regenerating transaction screen was added in 1.50.1 to fix #2288. It has been removed and I think not fully replaced by later commits ? Also is the comment above still accurate ?
There was a problem hiding this comment.
PS a little more context, my understanding was that the right fix for #2288 would be to make regenerateScreens take parent screens' state into account (fold rather than map).
| updatedTxn = case find (\t -> tindex t == tindex currentTxn) (jtxns j) of | ||
| Just t -> t | ||
| Nothing -> currentTxn -- fallback to current if not found | ||
| in tss { _tssTransaction = (currentIdx, updatedTxn) } |
There was a problem hiding this comment.
Does this replace the special regeneration previously in TransactionScreen.hs ?
Jumping to a different transaction after reload or file change is not ideal.
I think the old code exited to the register screen in that situation.
| RS rss -> RS $! rsUpdate (aopts ui') d j rss | ||
| TS tss -> TS $! tsUpdate (aopts ui') d j tss | ||
| ES _ -> s | ||
| in ui' |
There was a problem hiding this comment.
Screen types and functions are listed explicitly in a bunch of places, so maybe it's no great loss to do that here also - but is it possible to achieve the same strictness without listing them all ?
More seriously, doesn't it now only regenerate the current screen ?
There was a problem hiding this comment.
(Comments so far are based on code review, I haven't tested yet. Unfortunately it's easy to break behaviours in hledger-ui in ways that currently are hard to test, except manually.)
2 participants
Fixes #1825
... at least the increasing memory usage on each press of g is fixed by making new data structures and allowing the old data to be garbage collected. I'm not sure about CPU usage as that did not go up appreciably in my repro for this issue, and I haven't tested this long enough to know if it also solves the ‘leave running for days’ variant of the problem rather than the repeated manual refresh variant, but it seems likely it will.