<table><tr><td style="">fabiank created this revision.<br />Herald added a reviewer: KDE Games.<br />Herald added a subscriber: kde-games-devel.<br />fabiank requested review of this revision.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D23404">View Revision</a></tr></table><br /><div><strong>REVISION SUMMARY</strong><div><p>Why:<br />
As is evident from BUG 395624, we do have an issue with memory safety<br />
in KPatience. However, debugging it is really hard, and it is hard to<br />
find out the root cause.<br />
To ease further debugging, simplifying and modernizing the memory<br />
management seemed to be of utmost importance.</p>
<p>State before this patch:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">The TREE datastructure was used to store the state after a specific move. However, you would not know that by looking at it, as it only had a depth, left and right member. The actual data was always stored directly after it (as an array of PileBytes bytes). TREE actual data ------------------=-------------------- |depth|left|right|=Pile1|Pile2|Pile3... ------------------=-------------------- ^ ^ | |----was accesed via p+(sizeof(TREE)) | pointer p to here gets returned</li>
<li class="remarkup-list-item">The memory manager used a bump allocator which was used for both the POSITION and TREE structs. The bumb allocator used a simple linked list of blocks, which contained an array of data. The contents of that array looked like --------------------------------------------------------------------- |TREE1|Tree1Data|Tree2|Tree2Data |POSITION|TREE3|TREE3DATA|... ---------------------------------------------------------------------</li>
<li class="remarkup-list-item">To decide how much memory needs to be allocated, the size of the required memory was passed to the MemoryManager at runtime.</li>
<li class="remarkup-list-item">(De)allocations were made with C functions, so neither ctors nor dtors were called.</li>
<li class="remarkup-list-item">The MemoryManager stored and accessed state in static variables. Those were partially set by the concrete Solver instance on initialization.</li>
</ul>
<p>State after this patch:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">The MemoryManager, the TREE and datastructures involving TREE take PileBytes as a template argument. TREE has been renamed to TTree (templated tree), mostly to make finding usages easier.</li>
<li class="remarkup-list-item">As the TTree now knows how much data it has to store, it has an actual data member. Furthermore, in debug builds there is a canary value afterwards, which can be used to check for out-of-bound writes. ------------------------------------------------ |depth|left|right|data [PileBytes large]|CANARY| ------------------------------------------------ {--------------always there------------}={in debug bulids}</li>
<li class="remarkup-list-item">As the information is passed via template parameters, the static variables used for communicating sizes could be removed.</li>
<li class="remarkup-list-item">The bump allocator has been splitted, in one for TTrees and one for POSITIONs. They still share the total allocation limit. Splitting the allocator also allowdes for a now even simple bump allocator. It uses a simple array, and just returns a pointer to array[fill] upon request. If the array runs out of space, a new block gets allocated, just as before.</li>
<li class="remarkup-list-item">The blocks of the bump allocator now use unique_ptr, instead of having to manually release the memory at the end.</li>
<li class="remarkup-list-item">No code outside of the core memory allocation has been altered.</li>
</ul>
<p>Downsides:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">There is a somewhat larger initial memory footprint, as we are allocating a block for both POSITIONs and TTreees. However, in actual usage that should not matter, as the solver normally needs quite a lot of both of them, and would allocate a new block anyway.</li>
<li class="remarkup-list-item">The new bump allocator contains an array, which means that for each member the constructor gets called (previously the data was just memzeroed). However, except for debug builds, the constructors of TTree and POSITION are trivial, so there shouldn't be any work done.</li>
<li class="remarkup-list-item">(De)Allocations are now done with new and delete to ensure that ctors and dtors run.</li>
<li class="remarkup-list-item">All those templates aren't great for readability.
<br /><br />
Testing:<ul class="remarkup-list">
<li class="remarkup-list-item">Only manual testing to check whether the solver</li>
<li class="remarkup-list-item">The existing unit tests partially fail, but did so already before this change.</li>
<li class="remarkup-list-item">Application was run under sanitizers to check for new memory issues; none were found so far.</li>
</ul></li>
</ul></div></div><br /><div><strong>REPOSITORY</strong><div><div>R410 KPatience</div></div></div><br /><div><strong>BRANCH</strong><div><div>fixmemory</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D23404">https://phabricator.kde.org/D23404</a></div></div><br /><div><strong>AFFECTED FILES</strong><div><div>CMakeLists.txt<br />
autotests/CMakeLists.txt<br />
patsolve/memory.cpp<br />
patsolve/memory.h<br />
patsolve/patsolve.cpp<br />
patsolve/patsolve.h<br />
patsolve/solver_datastructures.h<br />
patsolve/solverinterface.h</div></div></div><br /><div><strong>To: </strong>fabiank, KDE Games<br /><strong>Cc: </strong>kde-games-devel<br /></div>