- User Since
- Feb 27 2018, 2:16 PM (98 w, 6 d)
Mon, Jan 13
Sorry, this fell by the wayside. I've had the results attached in a comment in https://phabricator.kde.org/D23507 for my system. I'll make the benchmark optional real soon™…
Moved function back so that the diff shows the actual change more cleanly
Nov 23 2019
Turns out that QDom actually won't go away, but we still need to check for any usage of deprecated APIs from QDomDocument
Nov 21 2019
While the change should be fine as is, that sounds like a job for std::deque, as we don't need insertions/removals at random positions. Or even for a std::queue, because we only operate on the front.
Sep 7 2019
(include BUG line)
Aug 29 2019
This is hopefully already a minimal fix for the crash issues, and it does not depend on Qt 5.12 features. @wbauer It would be most helpful if you could test whether the original bug is indeed no longer occurring after this patch.
Aug 28 2019
And this might have been the source of all problems (and adding it to the original code is probably sufficient, though I still think that the revamped memory management is a good idea): The FcSolveSolver did not call free, which left the MemoryManager in an unideal state, and would probably lead to (non-graciously handled) out-of-memory situations. This too should be fixed now.
- add missing call to free in FcSolveSolver::patsolve
The issue with the previous version was that the memory accounting code was wrong, and the allocation would fail at some point. The most recent version fixes this, and I could successfully complete a game of Grandfather's Clock again.
- fix memory accounting
Ah, I managed to reproduce the crash, and the interesting line of my backtrace is
#7 0x0000555a64cb306d in MemoryManager<19>::BLOCK<POSITION<19> >::getItem (this=0x0) at /home/fabian/projects/kpat/patsolve/memory.h:90
Thank you very much for testing the patch. Could you maybe compile kpat with
cmake -DCMAKE_BUILD_TYPE=Debug -DECM_ENABLE_SANITIZERS='address,leak,undefined' ..
and report back any backtrace you get from playing Grandfather's Clock? I again cannot reproduce the crash :-(
Aug 27 2019
Using the new benchmark, we can see some improvement, mostly in the single threaded case:
The micro benchmark is rather simplistic, but I hope still better than nothing (and I want it to show that D23507 makes sense)
Aug 26 2019
Aug 24 2019
The reflow in Phabricator's UI breks my ASCII diagrams, the git commit message should hopefully still be readable.
Oct 13 2018
Oct 12 2018
Oct 10 2018
Unfortunately, there are no unit tests at all, as there is a rather tight coupling of UI code and game logic. The RNG could however be tested in isolation. Though I'm not sure how one could test whether it behaves the same as Microsoft ones.
Thanks for the patch. I can't reproduce the crash, but I agree with you that this is UB and should be fixed. I don't think that this changes how the RNG works, but even if it would, not crashing is more important.
Sep 20 2018
Well, considering that it does look better now, and I'm not aware how to derive any better values, I'll say ship it. Do you have commit access?
This does indeed look better, and I didn't saw issues at screen resolutions I could test.
Aug 29 2018
Yikes, your reply went victim of an overzealous spam filter rule, sorry for the late reply. Considering that there are a few outstanding bugs like bko 397817, I think I will delay merging this for a little longer. I think I probably should first attempt to write some integration/unit tests, to ensure that it really works.
Jul 2 2018
@aacid : Ouch, I wasn't aware of that, and hoped that phabricator would do the right thing. I know how to do this correctly using git, but I have no clue how I get arcanist to preserve the author. Could you point me to the appropriate documentation, so that this mistake doesn't happen again?
Jun 30 2018
Jun 28 2018
Thanks for the heads-up Christoph, I'll look into the bug report. For reference, I'm simply “Fabian” on bko.
Jun 6 2018
May 31 2018
Well, thanks everyone for adding the required libraries, and sorry @bcooksley for not giving notice in advance.
May 27 2018
Oh my, I probably should have closed this. This was more intended to make testing https://phabricator.kde.org/D11844 possible, and the changes from there landed on KLook's qt5 branch. I deem the current Dolphin code not suited for upstream. KLook should probably changed so that it can be DBus activated and QProcess can be avoided, and applying this patch caused a few runtime warnings when using Dolphin if I recall correctly.
May 23 2018
I currently don't have any *BSD to test, but it still works on Linux, and the change seems sensible, so +2.
May 22 2018
Sigh, could have thought of this. Yeah, I'll open a ticket.
Oh well, time to get this merged. I didn't get *too* strong objection from the mailing list, so I intend to merge this as is. Thank you very much @shlomif for your patience and your code contribution.
May 16 2018
Yeah, sure, I'll send the mail.
May 14 2018
Sorry, I technically reviewed it already, but forgot to hit submit :-(. I'm happy with the current state of the code (sans that one copyright line), and the solver does a better job than the current one.
Apr 23 2018
Considering that currently no one (including me, the de-facto maintainer) really knows what the solver is doing exactly and how the numbers for the search in it were derived, I appreciate outsourcing the logic to a library. However, like @aacid said, it would be nice to have either some numbers how the solver compares.
Apr 8 2018
Yes, I have commit access. This should (arc) land in master?
Apr 7 2018
Apr 6 2018
I'm to tired to test this myself, but could you please test this with the edge case where some file names are so long that they don't fit in the view? Also, Stretch disables manual resizing, right?' If that is also deemed useful, we could adapt the solution from https://centaurialpha.github.io/resize-qheaderview-to-contents-and-interactive. That's pyqt, but the same idea applies to C++. The resize event handling code is even already there, just needs to be adjusted.