msohn

You're missing TObject.InitInstance which ensures all fields are initialised before the constructor is called. Edit: link for convenience https://docwiki.embarcadero.com/Libraries/Athens/en/System.TObject.InitInstance

I'm glad I took the time to explain how I read the VTune flame graph and that you tried and verified most of what I expected (well guessed tbh). And that you now saw first-hand why the original code was written the way it was. Welcome to the world of maintaining legacy projects😉 I'm sure you figured out how to get a copy of VTune, if not refer to the thread by Anders which I linked to in the flame graph post. It contained all I needed to get going. I read your post like you reverted your changes for now, but you might want to consider a few things which could improve the solution you tried: 1. Make sure all the loaded info is kept, i.e. when opening the dialog a second time, everything is already there. That way the user has to endure the delay only once. If that proves that memory usage is of no real concern (which I'd expect), you could go ahead and 2. Consider loading the info asynchronously, i.e. in a thread/threadpool as soon as the app starts - then, if the users opens the dialog before the loading has finished, show a progress and wait for it to finish. This should behave great when it works, but making that codebase thread safe is an enormous task, especially since there don't seem to be unit- and functional tests which help you make changes with confidence

Absolutely correct. To see where the time is spent, look at the VTune screenshot I posted here a few day ago: You have to read it from the bottom up. The x axis is time spent and it's only showing the time spent in ...BaseMenuScreen.DoLevelSelect, i.e. clicking the button to show the dialog. There's basically 3 main "pillars". The right-most shows all the way from TCustomForm.Show, via your TFLevelSelect.FormShow event handler calling SetInfo. The LoadNodeLabels part is what I was referring to in my last post where only visible, uninitialised nodes are updated. That is something that I would recommend joining with .InitializeTreeView - but do it for all nodes - and show a progress while you do it. The caching happens in LemLevelNeoPack, for example in TNeoLevelEntry.GetTitle. All these property getters call LoadLevelFileData, which checks LoadState telling it what was already loaded and then only load the missing parts or do nothing. Finally, to be clear: someone (you?) already put quite some effort into optimising the LevelSelect dialog, i.e. to only spend as little time as possible with filling the GUI initially (think InitializeTreeView) and then load all other parts upon user-interaction (SetInfo calls). It just turned out to be too slow (maybe too many levels/groups/packs now) and a mess (calling SetInfo at too many places). Now the task is to undo all of that, do the slower loading of everything while showing a progress and giving your applications window message pump more time to "breathe" and fix the freezing. The result hopefully is a better, more responsive compromise: opening the dialog the first time will take a lot longer than now - the progress will move slowly - but subsequently, a lot quicker.

It already does this. Again, I don't have the code at hand right now, but I remember something like a FLoadLevel which signalled what kind of data had already been loaded so you could call the method again, and if it already loaded the info, skipped the loading, i.e. a cache. So my idea would be to fully update the tree including all the loading in InitializeTreeView, i.e. you would merge SetInfo (edit: only the part that updates the tree) and InitializeTreeView and show a progress while you do that. That would also allow to to get rid of the OnExpanded node handler you added earlier in the thread here. Because of the above cache, it wouldn't actually reload it. Again, I might be wrong, if the user-data does not have the aforementioned cache. Also because e.g. the tree nodes are only updated once - for all future calls to SetInfo a node update is skipped if it's text is non-empty. That way all the initially blank nodes created in InitializeTreeView are updated only once, including image index etc. If any of the user data influences the node text or its image, then it wouldn't be properly updated. Again - might be wrong, can't check. Fair enough - this was merely directed at considering 3rd party tree views. Also because I'm still optimistic it isn't needed.

From my limited experience with the code, I would say this is definitely the approach to take. Bear in mind though that some information might actually get changed in the dialog (it does, right?) - I had the impression that there were already optimisations in place that would prevent this from working, but I did not get to the bottom of this, so I might be wrong. I would do this first - and only if the result is still not acceptable, would I consider other options like 3rd party trees. But I'm quite positive that won't be necessary with preloaded info. Also: since your end goal is a totally different UI without a tree, why bother? Actually - that UI works so different that maybe you wouldn't even need to cache info at all since there is a lot less info which is displayed once at a time.

I was looking for an excuse to use VTune again 😉 (thanks to Anders map2pdb!) and here's a flame graph of what's going on while the form is shown: (this is without any sounds being played in the background since I don't have BASS dll lying around - and I'm glad I don't) So the Sleep in ApplicationIdle wastes about one third of the total time. And while FormCreate does spend quite some time in InitializeTreeView, half of it is now because the OnExpanded handler is called during it (that's what @Lars Fosdal was hinting at). But in the end most of the time is spent reading and parsing the level info that is spread over hundreds of text files.

Not with the built-in tools, but here's an excellent summary by Remy of what you need to do: https://stackoverflow.com/a/1130506/386473 I just used exactly that approach and was able to find and fix the memory leaks in like half an hour. I'll try to make a pull request on GitHub as the patch is a bit larger due to spacing when I introduced new try/finally blocks.

Yes I see that now - didn't properly test my changes. Have a look at the attached patch - before exiting, it makes the talisman box now visible. Fixes the issue that you described. SuperLemmixPlayer-21-25-34.patch

Are you using a GUI git client? Mine offers to apply patches (I highly recommend https://fork.dev). But after all it's a text file, it's not that difficult to read and understand it, especially since the changes are small and I described what I changed. And then there's also the command line, see https://git-scm.com/docs/git-apply

@Willicious: have a look at the .patch I attached. That are 2 relatively small changes that improve the situation a lot from what I can see. It caches the group for which the PackTalismanInfo has been generated so it can skip that time-consuming process if the selected group is still the same. And it uses the .OnExpanded handler I mentioned above. SuperLemmixPlayer-18-27-08.patch

Yes, I see that now too. SetInfo really does too many things - it not only shows the details about the level/group, it also manages the tree (i.e. LoadNodeLabels) - but only the visible ones, and it does so every time, even if most of the nodes will end up unchanged because they were previously updated. Edit: I see now that it skips nodes with a non-empty text, but that renders it quite useless - I thought the whole point of doing it so often, i.e. in SetInfo, is to be able to react to changes? If there are no changes, when why not fill everything properly in InitializeTreeView? A better place for calling SetInfo might be in OnExpanded - as that is the only place where uninitialised tree nodes become visible (also AFAICS). I made that change (uncommenting the Click and DblClick handlers and adding a SetInfo call in a new Expanded handler) and it seemed to work OK! The whole fInfoForm seems very inefficient to me and it also doesn't really behave very user-friendly (no mouse-wheel support, only the buttons are clickable). The whole thing could be done as an owner drawn listbox with a bit of extra code to react to the "buttons". This will certainly take some effort, but it would improve things a lot since not all x00 entries need a button and two labels being created.

@Willicious: I managed to get all relevant code from github and have it compiling and running. How do I reproduce this? The only bit of a hang I've seen is when I click one of the larger level group nodes - then it takes quite a while because of hundreds of buttons and labels being created on the fly. And another thing which makes this worse, there are many updates, i.e. SetInfo calls, even though the selected tree node hasn't even changed - a simple cache of what was previously selected would improve that situation - or at least remove the Click and DblClick handlers which call SetInfo - any click will rebuild the GUI even though the selected node has not changed.

Well ApplicationIdle definitely contributed to your program not handling the window message queue in a timely manner. But for sure it might not be the only one contributing to it. If you keep ApplicationIdle commented out and repeat the steps to get the call stack again, what do you get? Also, is there anything unusual on the form being shown? Does it already take unusually long for the modal form to show up? Coming back to a previous point of mine, which you seemed to have misunderstood: are there any TActionList components in this project? Actions are updated with the same mechanism, i.e. from within TApplication.Idle. If that takes long, the symptoms would be very much the same.

From the documentation for OnIdle: https://docwiki.embarcadero.com/Libraries/Athens/en/Vcl.Forms.TApplication.OnIdle "Applications that set Done to false consume an inordinate amount of CPU time, which affects overall system performance." Your call stack shows that you clicked on an Image (no TreeView involved here), which causes some dialog to be shown modally. While this dialog is shown, your TGameMenuScreen.ApplicationIdle code is called continuously. I assume you've written the code for ApplicationIdle - can you describe what that code is intended to do? Because it's not obvious to me. Why the sleep? Why do you never set Done to True? Edit: also, it's quite telling, that you disable the OnIdle while you show a modal form in your ApplicationIdle handler - but you don't seem to do that in DoLevelSelect that is called as a result of clicking the image. I'd try that first, i.e. add DisableIdle and EnableIdle (ideally with a try/finally!) before/after your ShowModal call in DoLevelSelect.

Both the "program not responding" and the delay between clicking a tree node and hitting the breakpoint indicate that your program becomes very slow to process its window message queue. Unfortunately there are many reasons why this is the case. Some ideas that come to my mind are: 1. If you know in advance which node to click to experience the delay, press Pause ASAP after clicking and before hitting the breakpoint (a shortcut won't help, since while you click your program is active and not the IDE). Do this a couple of times to learn where your program typically spends that delay. If you end up in a similar spot in your code more than once, chances are you're looking at the cause (check the call stack!) 2. Do you use threads/tasks in connection with .Synchronize calls? Anything lengthy and blocking in synchronize has the potential to cause such effects. 3. Do you have Action(List) OnUpdate handlers that could take a bit longer? Depending on how complex your GUI is, these handlers might get called a LOT, so they should never do anything costly - if so, consider a caching mechanism. 4. Is this a normal VCL TTreeView? These are known to become quite inefficient with a couple of thousand nodes. Depending on what you do, maybe even with less than that. Of course these are all things to keep in mind even if you go down the OutputDebugString route.