Fix infinite memory usage by allowing old data to be garbage collected#2473
Fix infinite memory usage by allowing old data to be garbage collected#2473alerque wants to merge 4 commits intosimonmichael:masterfrom
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. |
alerque
force-pushed
the
ui-leak
branch
2 times, most recently
from
4740f83 to
82bbc97
Compare
simonmichael
left a comment
simonmichael
left a comment
There was a problem hiding this comment.
Nice strictness investigations. I have noted some questions.
| 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.
There was a problem hiding this comment.
Yes it is extra work but without the extra call to regenerateScreens here some of the data that used to be attached to the UI before the reload but hadn't been displayed yet and hence not fully processed are not released and they end up becoming dangling thunks. I'm sure this is not the most efficient way to handle this, but the alternative is a memory leak and my Haskell foo is not up to a better fix.
| -- 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).
There was a problem hiding this comment.
This PR does not regress #2288 if that's what your asking. It represents a bit more aggressive way of fixing that doesn't leave unused thunks that cannot be garbage collected.
No the comment is not accurate any more, I'll remove that too.
| 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.
There was a problem hiding this comment.
Yes, this does effectively do what the removed code was doing. I also agree jumping to a different transaction if the data changes on disk and the new data has different transaction indexing is not ideal, but this does not seem to be a regression. It does the same thing for me in 1.51.1 as well. Also both old and new versions fall pretty flat if the index they were looking at no longer exists at all. I think that would be a good case for a separate bug report though as it isn't a behavior regression and adding features while trying to chase down memory leaks doesn't sound like my idea of a good time.
| 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.)
There was a problem hiding this comment.
🤷♂️ Yes, these are listed in like half a dozen places, I'm sure that could be refactored. What wasn't working was the previous "smart" way of updating just a bit of the screen was orphaning unresolved data structures and accumulated thunks.
Yes, as I understand it, it is now only regenerating the current screen, but before it does that it forces a full copy of the journal data in memory so the old one can be garbage collected. Other screens will generate when you navigate to them, so my general impression is this is a more sound approach. I still think there are holes here and the whole UI feels upside down to me. That might be because the architecture really is kind of backwards or it might be I still haven't really gotten my head around Haskell. The smart money is on the latter, but it's probably some of both.
alerque
force-pushed
the
ui-leak
branch
2 times, most recently
from
a95408c to
e1b9cad
Compare
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.