#1910 Scite (on Linux) jammed on regexp replace

[Philippe Berthault]

Scite 3.7.2 compiled on Linux Fedora 20 (x86_64) with GTK+ 3.10.9.

When I want to replace a regexp on a file, Scite is jammed and must be killed with kill -9.
The expression I want to replace is "^.tfchan0 : " and the replacement string is empty.

The stack taken with gdb shows Scite is jammed on Document::CountCharacters method.

In attachment, the complete stack taken with gdb.
Best regards.

[Neil Hodgson]

I can't see a hang with this regular expression. Could you provide the file that this is occurring in?

The stack trace shows a sample inside the accessibility support code. I do not think this should be possible unless you have turned on the screen reader. Its also unlikely to cause problems unless the file is large and there are many substitutions. Try with a small file and with the screen reader (found in the Universal Access system settings) turned off and on.

[Philippe Berthault]

Because this problem was systematic on all files I edited with Scite, I rebooted my Linux station. After the reboot, the problem has disappeared and now I can't reproduce it.
So you can close this ticket.
Sorry, I don't known the reason but the problem was in Linux, not in Scite.
Best Regards.

[Philippe Berthault]

In fact, the problem has not disapperead. I can reproduce it. You wil find in attachment, a zipped text file with which I've always the problem when I want to replace the regexp "^.tfchan0; " by an empty string or a defined string.

I tried the latest SciTE version 3.7.3 and the problem is identical.
But with SciTE 3.7.0, there is no problem and all is good.

So, I think you introduced a regresssion in SciTE 3.7.1 with the new accessibility feature.
Best Regards.

[Colomban Wendling]

OK, I can reproduce the issue. There is no crash, deadlock or anything, it's just slow. The regular expression should be ^.*tfchan0: (or likely anything that leads to a lot of matches), replacement indeed doesn't matter.

What happens is that the accessible code is seeing each change, and counts character to emit proper notification, but the counting becomes the cullprint on a large file.

Unfortunately I don't exatcly know when the method that leads to the ScintillaGTKAccessible object to be create is called by GTK, it might be unconditionally on some setups -- if not most, when accessibility isn't wholly disabled.

I'm afraid this shows that we need a more performant way of getting the character offset. We can either make this a general-purpose way to improve CountCharacters() speed, or limit this to the ScintillaGTKAccessible by e.g. adding line-based cache there.

[Colomban Wendling]

or limit this to the ScintillaGTKAccessible by e.g. adding line-based cache there.

I just implemented caching on the accessible side, which while still resulting in a noticeable lag on such 62k replacements, is dramatically faster.

[Neil Hodgson]

The implementation of the cache in the patch appears to be enough for this issue since the revalidation area seems quite constrained. If it were to be made available on other platforms or for application use, it should invalidate less to avoid heavy (perhaps quadratic) costs. It should also use a reference count so that it could be enabled/disabled independently by the accessibility code and by the application.

It appears that accessibility is active even when the user does not have any accessibility features turned on. It should be possible to avoid the accessibility overhead with an option and I think this option should default to off. There is a warning if get_accessible returns NULL but the documentation says "If accessibility support is not available, this AtkObject instance may be a no-op."
https://developer.gnome.org/gtk3/stable/GtkWidget.html#gtk-widget-get-accessible

[Neil Hodgson]

I'll implement an option for enabling/disabling a11y.

[Neil Hodgson]

Attached is a patch that implements an option for enabling or disabling accessibility merged with the speed up patch from Colomban. Includes some traces to check functioning and appear to work OK when toggling accessibility from SciTE on current Ubuntu.

[Colomban Wendling]

I'm not fond of programatically enabling or disabling a11y as it's not the way GTK does it (apps don't have to do anything themselves normally), and it would require each app to opt-in to it, which most likely won't. OTOH I agree that the overhead shouldn't be there when the user doesn't need it.

I'd like to find out why currently the accessible object is even created (basically, why get_accessible() is called) on so many setups. I'll try and see to it as soon as possible.

Otherwise, technically the patch looks sensible (not tested yet though).

[Neil Hodgson]

There are quite good arguments on both sides of defaulting to accessibility enabled or disabled. Default enabling means that people who need accessibility will receive it with no effort in all applications. Default disabling means better performance which may be decisive in some cases - people often judge software by its performance out-of-the-box with no changed settings. This isue indicates that some will see the performance cost (of the original release not including the patch) as excessive and when one person writes a fault report, there will be others who can't be bothered or just move on to something else.

With the patch, the speed is improved but there is still some speed lost and there is a storage cost. People do complain about excessive memory use by Scintilla.

[Philippe Berthault]

I applied the patch and it's now OK for me on Linux Fedora, even with a very big file.
Thanks a lot for the speed you solved this problem.
Best Regards.

[Neil Hodgson]

The patch that adds the cache has now been committed as [e12538].
The API to enable accessibility has not yet been committed.

[Neil Hodgson]

Committed API [235468] for querying whether accessibility is enabled and, on GTK+, enabling or disabling it. The default state is that accessibility is enabled on GTK+. Also fix in [559dea].

[Colomban Wendling]

OK, good. For now I couldn't find any way to disable a11y objects in GTK3, so I guess it's as good as we can get unless I missed something.

Also fix in [559dea].

There seem to be another duplication in ScintillaGTKAccessible::Notify(): the cache clearing seems duplicated.

[Neil Hodgson]

OK [71bee2].