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

Freezing for a long time when replacing strings #2092

Closed
sm1ly opened this issue Feb 26, 2019 · 15 comments

Comments

Projects
None yet
6 participants
@sm1ly
Copy link

commented Feb 26, 2019

How to reproduce:
i=0 ; while [ "$i" -lt "60000" ] ; do echo '[20-01-2019 JUST TEXT. JUST TEXT. JUST TEXT. JUST TEXT. JUST TEXT. JUST TEXT. JUST TEXT. JUST TEXT.' >> testfile.txt ; i=$((i+1)) ; done

Next open testfile.txt
CTRL+H and replace [20-01-2019 with [21-01-2019 in whole document.
Now replace [21-01-2019 with [22-01-2019
It will freeze for a long time and load one cpu core for 100%. no ram or io load.
I set it for a night and it replaced all.

I use Fedora 29 and geany 1.34.1 and settings when replacing:
image

@codebrainz

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

Can't reproduce on Win10, Geany 1.34. Replacing all is nearly instant.

@sm1ly

This comment has been minimized.

Copy link
Author

commented Feb 26, 2019

Can't reproduce on Win10, Geany 1.34. Replacing all is nearly instant.

Can't reproduce it on Win10, Geany 1.34. too.
Ubuntu 18, Geany 1.32 - reproducing.

@LarsGit223

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

I can also reproduce it on Ubuntu 18.04.2, current development version (future Geany 1.35). Disabling all plugins does not help so it does not seem to be plugin related.

@LarsGit223

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

Some more info (all cases with @sm1ly's testfile):

Case 1:

  • Open testfile
  • do first find & replace
  • ok
  • do second find & replace
  • hangs

Case 2:

  • Open testfile
  • do first find & replace
  • ok
  • save changed file to disk
  • do second find & replace
  • hangs

Case 3:

  • Open testfile
  • do first find & replace
  • ok
  • save file, close it, re-open it
  • do second find & replace
  • ok
@elextr

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Tsk Tsk, nobody has provided their Glib, GTK versions :(

on

11:25:00: Geany INFO		: Geany 1.35 (git >= 4452b36), en_AU.UTF-8
11:25:00: Geany INFO		: GTK 3.18.9, GLib 2.48.2

I get the "hang" on first replace when running under GDB, stopped it a few times always got a bt like:

(gdb) bt
#0  0x00007ffff79c3663 in Scintilla::Document::NextPosition(long, int) const (this=this@entry=0x131a550, pos=pos@entry=1730705, moveDir=moveDir@entry=1)
    at src/Document.cxx:737
#1  0x00007ffff79c4cd0 in Scintilla::Document::CountCharacters(long, long) const (this=0x131a550, startPos=<optimised out>, 
    startPos@entry=1730700, endPos=1730800) at src/Document.cxx:1537
#2  0x00007ffff799b998 in Scintilla::ScintillaGTKAccessible::UpdateCursor() (byteOffset=5999900, this=0x14f0800) at gtk/ScintillaGTKAccessible.h:64
#3  0x00007ffff799b998 in Scintilla::ScintillaGTKAccessible::UpdateCursor() (this=this@entry=0x14f0800) at gtk/ScintillaGTKAccessible.cxx:799
#4  0x00007ffff799d0ae in Scintilla::ScintillaGTKAccessible::Notify(_GtkWidget*, int, SCNotification*) (this=0x14f0800, nt=0x7fffffffbe30)
    at gtk/ScintillaGTKAccessible.cxx:886
#5  0x00007ffff799d5f7 in Scintilla::ScintillaGTKAccessible::SciNotify(_GtkWidget*, int, SCNotification*, void*) (widget=<optimised out>, code=<optimised out>, nt=<optimised out>, data=<optimised out>) at gtk/ScintillaGTKAccessible.h:35
#9  0x00007ffff557d08f in <emit signal ??? on instance 0x136d0f0 [ScintillaObject]> (instance=<optimised out>, signal_id=signal_id@entry=430, detail=detail@entry=0) at /build/glib2.0-7ZsPUq/glib2.0-2.48.2/./gobject/gsignal.c:3441
    #6  0x00007ffff5561fa5 in g_closure_invoke (closure=0x14f0d10, return_value=return_value@entry=0x0, n_param_values=3, param_values=param_values@entry=0x7fffffffbb50, invocation_hint=invocation_hint@entry=0x7fffffffbad0)
    at /build/glib2.0-7ZsPUq/glib2.0-2.48.2/./gobject/gclosure.c:804
---Type <return> to continue, or q <return> to quit---
    #7  0x00007ffff5573fc1 in signal_emit_unlocked_R (node=node@entry=0x131a090, detail=detail@entry=0, instance=instance@entry=0x136d0f0, emission_return=emission_return@entry=0x0, instance_and_params=instance_and_params@entry=0x7fffffffbb50) at /build/glib2.0-7ZsPUq/glib2.0-2.48.2/./gobject/gsignal.c:3629
    #8  0x00007ffff557cd5c in g_signal_emit_valist (instance=<optimised out>, signal_id=<optimised out>, detail=<optimised out>, var_args=var_args@entry=0x7fffffffbd30) at /build/glib2.0-7ZsPUq/glib2.0-2.48.2/./gobject/gsignal.c:3385
#10 0x00007ffff79919af in Scintilla::ScintillaGTK::NotifyParent(SCNotification) (this=0x136c3b0, scn=...) at gtk/ScintillaGTK.cxx:1089
#11 0x00007ffff79db943 in Scintilla::Editor::NotifyModified(Scintilla::Document*, Scintilla::DocModification, void*) (this=0x136c3b0, mh=...)
    at src/Editor.cxx:2708
#12 0x00007ffff79c625e in Scintilla::Document::NotifyModified(Scintilla::DocModification) (this=this@entry=0x131a550, mh=...) at src/Document.cxx:2418
#13 0x00007ffff79c7cc3 in Scintilla::Document::InsertString(long, char const*, long) (this=0x131a550, position=223300, s=<optimised out>, 
    s@entry=0x15a0930 "[22-01-2019", insertLength=11) at src/Document.cxx:1250
#14 0x00007ffff79c7e2a in Scintilla::Document::InsertString(long, char const*, long) (this=<optimised out>, position=<optimised out>, s=s@entry=0x15a0930 "[22-01-2019", insertLength=<optimised out>) at src/Document.cxx:1212
#15 0x00007ffff79dc1d6 in Scintilla::Editor::ReplaceTarget(bool, char const*, long) (this=this@entry=0x136c3b0, replacePatterns=replacePatterns@entry=false, text=text@entry=0x15a0930 "[22-01-2019", length=11, length@entry=-1)
---Type <return> to continue, or q <return> to quit---
    at src/Editor.cxx:5565
#16 0x00007ffff79e1d0a in Scintilla::Editor::WndProc(unsigned int, unsigned long, long) (this=0x136c3b0, iMessage=2194, wParam=18446744073709551615, lParam=22677808) at src/Editor.cxx:6001
#17 0x00007ffff7997f8e in Scintilla::ScintillaGTK::WndProc(unsigned int, unsigned long, long) (this=0x136c3b0, iMessage=<optimised out>, wParam=<optimised out>, lParam=22677808) at gtk/ScintillaGTK.cxx:868
#18 0x00007ffff7963fe2 in sci_send_message_internal (file=file@entry=0x7ffff7b489fe "sciwrappers.c", line=line@entry=1088, sci=0x136d0f0 [ScintillaObject], msg=2194, wparam=wparam@entry=18446744073709551615, lparam=22677808)
    at sciwrappers.c:54
#19 0x00007ffff796577d in sci_replace_target (sci=<optimised out>, text=<optimised out>, regex=<optimised out>) at sciwrappers.c:1088
#20 0x00007ffff796bb13 in search_replace_range (sci=sci@entry=0x136d0f0 [ScintillaObject], ttf=ttf@entry=0x7fffffffc460, flags=flags@entry=(unknown: 0), replace_text=replace_text@entry=0x15a0930 "[22-01-2019") at search.c:2261
#21 0x00007ffff792b592 in document_replace_range (doc=<optimised out>, find_text=<optimised out>, replace_text=0x15a0930 "[22-01-2019", flags=(unknown: 0), start=<optimised out>, end=<optimised out>, scroll_to_match=1, new_range_end=0x0)
    at document.c:2507
#22 0x00007ffff792d568 in document_replace_all (doc=doc@entry=0x1366a70, find_text=find_text@entry=0x1586140 "[21-01-2019", replace_text=replace_text@entry=0x15a0930 "[22-01-2019", original_find_text=original_find_text@entry=0x1523360 "[21----Type <return> to continue, or q <return> to quit---

Seems suspicious that its always that Accessibility stuff counting characters, @b4n !!!!!!!!!!!!!!!!!!!!

@elextr

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Adding:

	SSM(sci, SCI_SETACCESSIBILITY, 0, 0);

in editor.c:create_new_sci() allows me to do the replace at least 5 times with no problem.

@elextr

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

@b4n, maybe make accessibility an option, after all its noted in Scintilla docs that it has performance impacts.

@b4n

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

@elextr yeah I figured the same late yesterday (for what seems to be the cause).
I'd rather not disable accessibility by default because it kind of makes it irrelevant if people need to opt-in, but I'd really like to be able to notice when it'd be useful and only enable it then (if there's a client consuming the info). We could have an opt-out option at least for testing though.
In any case I'll try and see if we can't optimize this a bit in some way or another, because cases like that a really problematic.

@kugel-

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

So is this dependent on the GTK version?

@b4n

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

So is this dependent on the GTK version?

shouldn't be, no

@elextr

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

but I'd really like to be able to notice when it'd be useful and only enable it then

@b4n, better to not try to be smart, it will always be wrong somewhere, users are too good at finding new things to do that we never thought of.

For sure play with improving things or trying to be smart.

But add a simple get out option too, with the default of on of course.

@b4n

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

@b4n, better to not try to be smart, it will always be wrong somewhere, users are too good at finding new things to do that we never thought of.

hehe yeah they are. However I found a not-really-smart (yay) but pretty nice technique based on g_signal_has_handler_pending() to prevent most of the computation when it won't be consumed anyway. I need to check if it doesn't break the accessible when it's actually used, but it's pretty promising for now (e.g. with my initial attempt I can do the replaces mentioned here with a much more reasonable delay -- a couple of seconds).

It still doesn't help the case where accessibility is used, that'll be a matter of optimizing the position-to-character conversion. Fortunately AFAIK there's position cache available in latest Scintilla that the accessibility layer could use. We'll see.

@b4n

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

I need to check if it doesn't break the accessible when it's actually used

Meh, it doesn't work. Apparently nothing is listening on the signal, which makes sense as it's forwarded through IPC… damned, I'll need to get another idea.

b4n added a commit to b4n/geany that referenced this issue Mar 2, 2019

scintilla: GTK: a11y: Optimize byte to character offset conversion
Instead of counting characters from the start position (which is most of
the time the start of the document), take advantage of our character
offset cache to reduce the range to actually count.

This gives drastic performance improvements, very noticeable during bulk
search-and-replace with a lot of matches.

Part of geany#2092.

b4n added a commit to b4n/geany that referenced this issue Mar 2, 2019

scintilla: GTK: a11y: Avoid clearing cache when possible
Instead of clearing invalid cache, update it right away to avoid
costly calls to Document::CountCharacters().

This gives drastic performance improvements with large documents when
the position cache gets cleared very often and a position near the end
is periodically computed.  For example, this happens when performing
bulk search-and-replace with a lot of matches while having the cursor
near the end: upon each replace, the cursor character offset is
requested again, and the replace operation invalidated the cache. If
the cache is cleared, this leads to a whole re-computation of the
cursor offset on every replace.

Fixes geany#2092.
@b4n

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

OK, I have a fix for making a11y quite a lot faster in this case, see #2097. It's still "slow", but it's now a matter of seconds, not tens of minutes or more.

It still might be good to only call all this when it's actually useful, although that'd mean a lot less testing -- and issues like that wouldn't be noticed as much.

@b4n b4n added this to the 1.35 milestone Apr 5, 2019

@b4n

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

I improved the patch a little further, and now I can hardly measure a difference between the first and second replacement times.

b4n added a commit to b4n/geany that referenced this issue 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 geany#2092.

@b4n b4n closed this in #2097 Apr 17, 2019

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.