Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scintilla: faster a11y #2097

Merged
merged 1 commit into from Apr 17, 2019

Conversation

Projects
None yet
3 participants
@b4n
Copy link
Member

commented Mar 3, 2019

2 changes to make a11y quite a bit faster especially when performing bulk search & replace.

The first commit makes use of the character offset cache when performing reverse-lookup, e.g. when we need to convert a position given by the a11y layer to one understood by Scintilla.

The second avoids clearing the cache when it gets invalidated, and instead updates it to be valid right away. This avoids potentially costly re-computation of the cache that becomes problematic when we often need a position near the end of the file but keep invalidating everything after lines near the start. This happens especially when performing bulk search & replace with the cursor near the end. This gives a slightly higher cost at invalidating the cache (that now updates it based on a known delta), but a lot less when it's content is needed again.

Fixes #2092.

@b4n b4n added this to the 1.35 milestone Mar 3, 2019

@b4n b4n self-assigned this Mar 3, 2019

@elextr

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

Scintilla is a separate project to Geany, please submit Scintilla changes there, preferably to both Scintilla 3 and 4..

The Geany maintainer likes to be able to update Scintilla by the requisite script and adding patches is not part of that.

PS 😁

@b4n

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

Oh sorry. I though maybe you guys would like a chance to test this fix and see if it does help #2092, but I can indeed submit it first to Scintilla if you guys prefer.

PS: fair enough 😄

@elextr

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

I though maybe you guys would like a chance to test this fix and see if it does help

Yeah, I figured that was why, but the automatic response was just too strong 😄 . I can't test just now, but maybe the OP can.

@b4n

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

Improved it further to simply use Scintilla's built-in position cache which is actually faster, and makes the code simple. Submitted upstream: https://sourceforge.net/p/scintilla/bugs/2094/.

@johndescs

This comment has been minimized.

Copy link

commented Apr 5, 2019

Fixes issue for me. Fixes also slow undo.
I also noticed that the first iteration is slow too when done with cursor at the end.
BTW this issue was not present (for me) in stock geany 1.29 from Debian Stretch (built with GTK+2).

@b4n

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

@johndescs thanks for testing!

BTW this issue was not present (for me) in stock geany 1.29 from Debian Stretch (built with GTK+2).

It's to be expected, as the offending code appeared in 1.30. But it might also be affected by the GTK version, as the code for instantiating the accessible object is fairly different on GTK2, and IIRC might disable it if the toolkit isn't currently enabling it somehow. Might be interesting to check, but shouldn't change too much when a11y is actually enabled.

@b4n b4n force-pushed the b4n:scintilla/faster-a11y branch from ea81d46 to b060cf5 Apr 17, 2019

scintilla: Accessible: use the built-in character position cache
It's quite a lot faster even after trying and optimizing the custom
version, and it makes the code simpler.

Also improve ByteOffsetFromCharacterOffset() to make use of the cache,
making it drastically faster.

X-Scintilla-Bug-URL: https://sourceforge.net/p/scintilla/bugs/2094/
X-Scintilla-Commit-ID: 01aab5f24e50ed14551c8c9c8ecce7ece0594c09
X-Scintilla-Commit-ID: 2c8b52af4ae5de2abe7c00fd18e78be60340cbf9

Fixes #2092.

@b4n b4n force-pushed the b4n:scintilla/faster-a11y branch from b060cf5 to a587385 Apr 17, 2019

@b4n b4n merged commit a587385 into geany:master Apr 17, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@b4n

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

Merged the version that made it to upstream Scintilla. It's the same with an additional check which only matter when Scintilla is not in Unicode mode, so doesn't affect us.

@elextr

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Merged the version that made it to upstream Scintilla.

Why? Its in Scintilla 3.10.4 that was released 10 minutes ago (at time of writing), why not just upgrade to that (subtle hint)? Weren't you able to see into the future to know that was going to happen? 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.