RomanYankovsky
Added bulk object allocator (memory pool) for syntax nodes
This branch implements a memory pool for each syntax node class. It means that nodes are allocated not from FastMM, but from a pool. When freed, the memory is returned to the pool, but the large allocation is not freed; in other words, repeated allocations and frees will not fragment memory. This comes at the cost of slightly higher memory usage for the life of the process (typically a few hundred kilobytes.)
This is turned off by default, and when off has absolutely no effect on the code.
This is part of my quest to reduce memory fragmentation in pre-XE8 IDEs for Navigator and Bookmarks. They re-parse files with almost every change (with a small delay) and over many hours this can result in many, many allocations and frees of the syntax node classes. Since other memory operations occur in between these, and since the pre-XE8 IDE has a small address space (2GB) further exacerbated by .Net's allocator sitting in the same process, fragmentation can occur.
This is not necessary for occasional use of DelphiAST, eg running it once. It may help significantly when run many times in a row, as in the Navigator/Bookmarks use case or when parsing several thousand file in a large project.
Thans, @vintagedave !
But FastMM uses memory pool too, doesn't it? I mean do you have any benchmark that shows that your memory managment is better than FastMM? It's very sensitive area, we have to be bery careful.
That's tricky. I have empirical evidence, which is memory stability in the IDE. I haven't done a full test which would require tracking all FastMM allocations and free areas of memory.
FastMM cannot ever optimize for a specific class. It can only optimize by size, and it does have pools per memory size (small, medium, and I think it goes straight to VirtualAlloc for large.) My suspicion is that the fragmentation is occurring at a higher level, eg it may be releasing entire blocks back to the OS and then re-acquiring them, so the fragmentation is at that level. It will always free memory, whereas this code does not - it is static pool that never shrinks, which means fragmentation cannot occur because there is no repeated releasing and reacquisition.
For my purposes (IDE plugins) this works and seems to be important (there is a noticeable stability increase in the IDE, especially older versions), so I will keep this in a private fork even if it's not merged. Personally I suspect this would benefit FixInsight too, if you parse large (say, thousand-unit-plus) projects. Note that it's not enabled by default and if merged in, no-one using it will notice any change because unless defined and built with the define, there is no code change.
I'm OK to merge this. I just want to be 150% sure, that's why I'm asking.
As far as I understood, this pull request includes StringCache, so if I merge this, previous pull request is not needed. Right?
Let's wait a couple of days, so everyone can say his opinion. @Wosi and @sglienke do intensively use DelphiAST, so I want them to be happy too.
Yes, I branched from my string cache branch, since it's part 2 of the memory fragmentation code. The string cache is IMO the most important of the two.
On 23 April 2016 at 16:23, Roman Yankovsky [email protected] wrote:
I'm OK to merge this. I just want to be 150% sure, that's why I'm asking.
As far as I understood, this pull request includes StringCache, so if I merge this, previous pull request is not needed. Right?
Let's wait a couple of days, so everyone can say his opinion. @Wosi https://github.com/Wosi and @sglienke https://github.com/sglienke do intensively use DelphiAST, so I want them to be happy too.
— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/RomanYankovsky/DelphiAST/pull/173#issuecomment-213742435
To be honest - using a simple object pool would have been enough where you request objects from and then put them back into. If you preallocated n object imo you have the same effect. Messing with the InitInstance and allocating memory yourself that you use by those objects seems unnecessary overkill and a possible source of errors (is it thread-safe? I don't think so)
It's not threadsafe, no. Of course, it's not used unless you turn it on - the code, when compiled with the define undefined, is absolutely unchanged from the current code.
The main thing with reusing objects in an object pool instead of a memory pool is ensuring that they are initialised correctly. With this technique, the constructor etc runs as normal, and memory is zeroed. With a reused object, you need to ensure that everyone one is used, all fields are reset. Without an extensive code inspection, I'm not confident of this in DAST since the code relies on new objects being in the state they are constructed to, ie assumes default without setting everything explicitly.
Resetting fields is done by calling TObject.CleanupInstance and initializing by calling TObject.InitInstance. Managing the objects lifetime and memory is not the purpose of the objects/classes themself but by someone else - of course you then have to refactor a fair bit of the DAST code to not just call Create/Free on these classes/objects but use an allocator/factory/objectpool (you name it).
But to me this would be the cleaner approach and easier to maintain when moving to a more typed node tree in the future because then you don't need to put that custom allocation code into every node class.
Hmm. Well, I could certainly rework this to do that instead. That would be a better option?
I have been looking into minimizing heap allocations by refactoring several methods in TPasSyntaxTreeBuilder. Also I refactored the node classes in DelphiAST.Classes to avoid some overhead which comes from InitInstance and CleanupInstance (which we don't need if we clear all fields ourselves). This refactoring has not been finished but is part of a larger performance improvement project of DelphiAST.
So this can be closed imo as the changes are quite outdated by now.