Use mutable variables to store information about last call to positionFromCursor.
As most of the time the cursor is close to the previous position, avoid to calculate every time the size of the lines from the beginning of the document, just calculate it from the previous cursor position.
Details
Details
- Reviewers
cullmann - Group Reviewers
Kate Frameworks - Commits
- R39:81996f55b1ab: [ktexteditor] much faster positionFromCursor
Open a callgrind log file with more than 1.00.000 lines,
go to the end of the file, and then play with the PageUp, PageDown and cursor keys.
Before: it was unable to do continuous PageUp refreshes.
After: the PageUp at any part of the file is as fast as in the beginning of the document.
Diff Detail
Diff Detail
- Repository
- R39 KTextEditor
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
Hi,
I appreciate the intend to speed this up.
But I think, to do this proper, it might make more sense to cache the current position in the KateViewAccessible and invalidate it on changes to the document.
The way you do it now, it might not be correct if text changes occur in-between. (if I don't overlook something)
Comment Actions
Cached the position in static variables of KateViewAccessible.
The cache is invalidated when the signal Document::textChanged is received.
Unfortunately, KateViewAccessible must inherit also QObject or connect will not work, because KateViewAccessible didn't inherit QObject.
Is this change Binary compatible?
At first I thought the changes will come in setText, but in my tests it has never been called.
To check the correctness of the result, I've run the fast and slow paths in kate, with several windows over the same document, changing it in sereveral ways, and the results always match.
Comment Actions
It can't be a member object, because some methods that call positionFromCursor are const. I tried :-(
Comment Actions
Implemented now as object method instead of static.
Changed the calls from KateViewInternal from static to use the reference the method QAccessible::queryAccessibleInterface(this) returns.
Comment Actions
Re. binary compatibility: should be fine because this class is not exported (no KTEXTEDITOR_EXPORT macro).
Comment Actions
Oh and, you do not need to inherit QObject to use connect; you can connect to a lambda calling the member function AFAIK or so. Just omit the third argument in connect(). What you lose by doing this is the automatic disconnect of the connection when the receiver object is deleted, so make sure that doesn't happen.
Comment Actions
No QObject inheritance, only a QMetaObject::Connection member to disconnect the connection.
As this is a non exported class, use the same method name and make it non-static.
KateViewInternal::cursorMoved checks if QAccessible::isActive().
Comment Actions
some nits, otherwise lgtm assuming all tests pass
bonus points for a proper benchmark that shows the before/after gain in hard numbers
| src/view/kateviewaccessible.h | ||
|---|---|---|
| 191 ↗ | (On Diff #31733) | I know the old code used int already, but shouldn't this better be a quint64 as it's a file offset? |
| 192 ↗ | (On Diff #31733) | const auto *doc = |
| 204 ↗ | (On Diff #31733) | style: join with next line |
| 215 ↗ | (On Diff #31733) | dito |
| 231 ↗ | (On Diff #31733) | unrelated change? |
| 270 ↗ | (On Diff #31733) | maybe also mention that this is a file offset? |
Comment Actions
Included more comments.
Addressed style coding.
In a callgrind log file with 1606189 lines, at the end of the file, pressing PageUp, using the code included at the end:
old implementation elapsed = 122
new implementation elapsed = 0
old implementation elapsed = 119
new implementation elapsed = 0
After some insertions:
old implementation elapsed = 137
new implementation elapsed = 118
old implementation elapsed = 123
new implementation elapsed = 0
--- src/view/kateviewaccessible.h +++ src/view/kateviewaccessible.h @@ -29,6 +29,8 @@ #include <QAccessible> #include <KLocalizedString> #include <QAccessibleWidget> +#include <QElapsedTimer> +#include <QDebug> /** * This class implements a QAccessible-interface for a KateViewInternal. @@ -194,6 +196,9 @@ public: */ int positionFromCursor(KateViewInternal *view, const KTextEditor::Cursor &cursor) const { + QElapsedTimer t; + t.start(); + int pos = m_lastPosition; const auto *doc = view->view()->document(); @@ -229,6 +234,22 @@ public: m_lastCursor = cursor; m_lastPosition = pos; + qWarning() << "new implementation elapsed = " << t.restart(); + // previous implementation + int _pos = 0; + for (int _line = 0; _line < cursor.line(); ++_line) { + // length of the line plus newline + _pos += view->view()->document()->line(_line).size() + 1; + } + _pos += cursor.column(); + qWarning() << "old implementation elapsed = " << t.elapsed(); + + if (_pos != (pos + cursor.column())) { + qWarning() << "implementations differ, old=" << _pos << " new=" << +pos + cursor.column(); + } + return pos + cursor.column(); }
Comment Actions
| src/view/kateviewaccessible.h | ||
|---|---|---|
| 191 ↗ | (On Diff #31733) | All the methods of QAccesibeTextInrface expect an int. |
Comment Actions
lgtm, @cullmann ?
regarding the accessibility interface, I agree that it's out of the scope of this patch. I'd say let's keep it like that for now...
Comment Actions
I am ok with it as it is ;=)
I would have been ok with the version that still inherits from QObject, too, but now that one has done all the hassle to avoid that ok ;)
Comment Actions
So design is broken,for me. Function should be static and cursor from view (KateViewInternal) should be compared against cursor parameter.
Comment Actions
Hmm, I don't understand that comment?
Now you have one (or more) object per view, or?
That looks sane to me.
Or do I misunderstand how that works?
Comment Actions
Maybe i miss something, but i think if something should be extended it's view not accessible.
Comment Actions
Do you mean to extend KateViewInternal in such a way that positionFromCursor in KateViewAccessible becomes unnecessary?
Comment Actions
Actually, given the functionality is only used there, if that code is in the view or in that helper class makes no real difference, or?
Comment Actions
I've tried to extend KateViewInternal in such a way that positionFromCursor in KateViewAccessible becomes unnecessary, but it became a mess.
The problem is that all the "coordinates" used there are Cursors, that are in the form line+column.
But in KateViewAccessible it uses characters from the beginning, and caching only in one place (positionFromCursor) does not create a mess.
Comment Actions
If the stuff still works for you and is fast now, I am still for merging that.
If somebody has time to do a even nicer integration, a new request can be done afterwards.
Comment Actions
I get the following warning from valgrind because of this change:
==3621== Conditional jump or move depends on uninitialised value(s) ==3621== at 0x4FF3605: KateViewAccessible::positionFromCursor(KateViewInternal*, KTextEditor::Cursor const&) const (kateviewaccessible.h:201) ==3621== by 0x4FE41CA: KateViewInternal::documentTextInserted(KTextEditor::Document*, KTextEditor::Range const&) (kateviewinternal.cpp:3830) ==3621== by 0x967762A: call (qobjectdefs_impl.h:376) ==3621== by 0x967762A: QMetaObject::activate(QObject*, int, int, void**) (qobject.cpp:3754) ==3621== by 0x50C25E6: KTextEditor::DocumentPrivate::textInserted(KTextEditor::Document*, KTextEditor::Range const&) (moc_katedocument_6APBFLF6VF33HC.cpp:985) ==3621== by 0x4F498DE: KTextEditor::DocumentPrivate::editInsertText(int, int, QString const&) (katedocument.cpp:1235) ==3621== by 0x4F4DF67: KTextEditor::DocumentPrivate::insertText(KTextEditor::Cursor const&, QString const&, bool) (katedocument.cpp:780) ==3621== by 0x4F51838: KTextEditor::DocumentPrivate::typeChars(KTextEditor::ViewPrivate*, QString const&) (katedocument.cpp:3024) ==3621== by 0x4FE9BF4: KateViewInternal::keyPressEvent(QKeyEvent*) (kateviewinternal.cpp:2522) ==3621== by 0x4FF1F12: KateViewInternal::eventFilter(QObject*, QEvent*) (kateviewinternal.cpp:2370) ==3621== by 0x96493EC: QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) (qcoreapplication.cpp:1174) ==3621== by 0x8284834: QApplicationPrivate::notify_helper(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.11.0) ==3621== by 0x828D30E: QApplication::notify(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.11.0) ==3621== Uninitialised value was created by a heap allocation ==3621== at 0x4C2C25F: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==3621== by 0x4FE3EF0: accessibleInterfaceFactory(QString const&, QObject*) (kateviewaccessible.h:288) ==3621== by 0x8A70D2C: QAccessible::queryAccessibleInterface(QObject*) (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.11.0) ==3621== by 0x8A7172E: QAccessibleEvent::accessibleInterface() const (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.11.0) ==3621== by 0x8A71BFC: QAccessible::updateAccessibility(QAccessibleEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.11.0) ==3621== by 0x82C0A72: QWidgetPrivate::show_helper() (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.11.0) ==3621== by 0x82C3CC4: QWidget::setVisible(bool) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.11.0) ==3621== by 0x82C0957: QWidgetPrivate::showChildren(bool) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.11.0) ==3621== by 0x82C09DE: QWidgetPrivate::show_helper() (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.11.0) ==3621== by 0x82C0946: QWidgetPrivate::showChildren(bool) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.11.0) ==3621== by 0x82C09DE: QWidgetPrivate::show_helper() (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.11.0) ==3621== by 0x82C3CC4: QWidget::setVisible(bool) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.11.0)