Hi! I found a fix for a specific startup-speed regression affecting Geany master. It may or may not have anything to do with what people experienced in #2649, but it can be reproduced as follows: give Geany a session of ~300 open C files to restore, and otherwise a default configuration, and simply start it. Geany master: ~12-24s. Geany 1.35: ~2-4s.
This is all moot if the Scintilla v5 upgrade (#2824) is imminent, as the cause is a mistake backporting some v5 code to the LongTerm3 branch. It started with https://github.com/geany/geany/commit/4bceddb449b8727e1d8b449a5d92af5fa53a24..., was caused by http://hg.code.sf.net/p/scintilla/code/rev/6c453822904a, and can be fixed with a 1-line change I posted to https://sourceforge.net/p/scintilla/bugs/2280/. Per Neil's comments there, further 3.x releases are unlikely, so Geany's options are to either adopt the fix directly or press on with the v5 upgrade.
Please let me know if a PR with the 3.x fix would be useful. (Will any releases happen before the v5 upgrade is done?) Best, Justin
Will any releases happen before the v5 upgrade is done?
It is likely that the release will happen in a few weeks, and since we _really_ don't want to have to release a fixup due to the difficulty in getting the release team together, I suggest a change like Scintilla v5 probably won't happen before due to the size of the change.
Having looked at your change, its not just about speed, but correctness.
As it is now the compare compares the two pointers, not the string contents. I'm not sure what Unique_string is used for, but its always going to fail to find the string and make a new one, so the list will get long and the compares get slow since it will do a linear compare through the whole list every time, and if the usage actually relies on uniqueness then it will possibly fail.
So it probably should be applied in any case if somebody made a pull request.
Great, thanks!
Closed #2883 via #2884.
Thanks!
Any info on how this relates to #2747? Can we drop it with your patch, or are they different issues? I'd love it if I can drop most of #2747 :)
Hi, interesting question! After testing, I don't know if #2747 is still beneficial or not (whereas this would have been obvious before). It looks to me like you could safely remove it if you wanted to.
Our changes superficially have nothing to do with each other, but this `UniqueStringSet` was previously named `FontNames`, so my guess is that an explosion of duplicate font-related data was the main reason for `SCI_TEXTHEIGHT` being expensive.
Here's an unscientific test where I ran `time geany` 5 times (on my laptop, while spamming ctrl-q), and averaged. So don't believe the table's precise-to-the-millisecond numbers. ;) When I say "no 2747", I mean I reverted its commits via `git diff 01604687a e027e240c279040c2f6bc9ea0aaccc2cac2ca94e | patch -p1 -R`.
| | master | no-2747 | no-2883 | neither | |------|--------|---------|---------|---------| | #1 | 2.718 | 2.781 | 9.310 | 26.806 | | #2 | 2.701 | 2.861 | 9.663 | 34.792 | | #3 | 2.638 | 2.567 | 9.336 | 34.179 | | #4 | 2.644 | 2.618 | 9.396 | 34.625 | | #5 | 2.670 | 2.889 | 12.187 | 34.638 | | mean | 2.674 | 2.743 | 9.978 | 33.008 |
Those startup times before #2747 look really painful. Nice work getting them under control, even if you decide against that approach in the end. :)
@UncombedCoconut one month later, but thanks a lot for trying some numbers (in addition to fixing the issue in the first place)! I'll see if I can find the time to measure it a tiny bit more consistently (inserting a timer around the relevant parts), but your numbers suggest that #2747 isn't useful anymore (which, despite the effort I put into, I'd be happy to get rid of). Anyway, it doesn't seem like having it in slows anything down measurably, so there's no hurry -- but not remembering what it was all about.
@UncombedCoconut one month later, but thanks a lot for trying some numbers (in addition to fixing the issue in the first place)! I'll see if I can find the time to measure it a tiny bit more consistently (inserting a timer around the relevant parts), but your numbers suggest that #2747 isn't useful anymore (which, despite the effort I put into, I'd be happy to get rid of). Anyway, it doesn't seem like having it in slows anything down measurably, so there's no hurry -- but not remembering what it was all about.
IMO, procrastination was a wise choice: A tested, known-good version 1.38 got released, and the Scintilla version on master is about to be 5.1.x. Once that's merged, I think it makes sense to re-test whether a backout of #2747 has any effect on performance.
Scintilla 5.1.1 is merged.
github-comments@lists.geany.org