Review Request 128480: fix clang plugin highlighting screwup
Sven Brauch
mail at svenbrauch.de
Thu Jul 21 07:33:54 UTC 2016
> On July 20, 2016, 10:17 p.m., Milian Wolff wrote:
> > I agree, let's go for it. And we all should report if we still reproduce the bug, or not (also, remember to reference it in your commit message, and close it for now).
> >
> > Could you do me a favor and have a look at the maximum resident set sizes reported by
> >
> > CLEAR_DUCHAIN_DIR=1 \time -v kdevelop
> >
> > before and after this patch, for a session that has a bunch of files opened? I simply wonder whether we now consume gigs of memory more, or just a couple of megs (I think the latter).
>
> Sven Brauch wrote:
> Not much of a difference.
>
> After:
>
> User time (seconds): 731.34
> System time (seconds): 15.96
> Percent of CPU this job got: 536%
> Elapsed (wall clock) time (h:mm:ss or m:ss): 2:19.30
> Average shared text size (kbytes): 0
> Average unshared data size (kbytes): 0
> Average stack size (kbytes): 0
> Average total size (kbytes): 0
> Maximum resident set size (kbytes): 1872812
> Average resident set size (kbytes): 0
> Major (requiring I/O) page faults: 0
> Minor (reclaiming a frame) page faults: 2952649
> Voluntary context switches: 133801
> Involuntary context switches: 60886
> Swaps: 0
> File system inputs: 88
> File system outputs: 251952
> Socket messages sent: 0
> Socket messages received: 0
> Signals delivered: 0
> Page size (bytes): 4096
> Exit status: 0
>
>
> Before:
>
> User time (seconds): 770.12
> System time (seconds): 16.27
> Percent of CPU this job got: 366%
> Elapsed (wall clock) time (h:mm:ss or m:ss): 3:34.68
> Average shared text size (kbytes): 0
> Average unshared data size (kbytes): 0
> Average stack size (kbytes): 0
> Average total size (kbytes): 0
> Maximum resident set size (kbytes): 1900812
> Average resident set size (kbytes): 0
> Major (requiring I/O) page faults: 1
> Minor (reclaiming a frame) page faults: 2485091
> Voluntary context switches: 228263
> Involuntary context switches: 93074
> Swaps: 0
> File system inputs: 1760
> File system outputs: 260736
> Socket messages sent: 0
> Socket messages received: 0
> Signals delivered: 0
> Page size (bytes): 4096
> Exit status: 0
I think I mixed up before/after. But not much of a difference in memory consumption anyways, as one would expect.
- Sven
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128480/#review97678
-----------------------------------------------------------
On July 20, 2016, 10:36 p.m., Sven Brauch wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128480/
> -----------------------------------------------------------
>
> (Updated July 20, 2016, 10:36 p.m.)
>
>
> Review request for KDevelop, Kevin Funk and Milian Wolff.
>
>
> Repository: kdevelop
>
>
> Description
> -------
>
> Terrible patch, I know. Sorry. It cost a lot of nerves and time though and I want somebody to look at / try out whether this is a working fix in principle.
>
> In the end, the issue(s) are simple:
>
> 1. we need to hold the foreground lock between reading the modification revision and reading the contents. Otherwise they can mismatch.
>
> 2. in buildDUChain(), there was
>
> envFile->setModificationRevision(ModificationRevision::revisionForFile(context->url()));
>
> which is wrong. It sets the modification revision of the parsing environment file to the revision the file has _right now_. The contents, however, were read before in the constructor, and several milliseconds might have passed until this line is reached, with no locks held to prevent anything.
> Instead, we have to set the revision to the one which was stored when the tracker was last reset (which happened right after reading the contents, coupled to the contents by the foreground lock).
>
> Actually I'm not sure if the foreground lock is required in this case, because clang for some reason reads the contents in the constructor, which afaik runs in the main thread anyways.
>
>
> Diffs
> -----
>
> languages/clang/clangparsejob.cpp 8375eb5
>
> Diff: https://git.reviewboard.kde.org/r/128480/diff/
>
>
> Testing
> -------
>
> Very hard to reproduce. Apply this patch to kdevplatform:
>
> diff --git a/language/backgroundparser/documentchangetracker.cpp b/language/backgroundparser/documentchangetracker.cpp
> index 294bc14..6f56250 100644
> --- a/language/backgroundparser/documentchangetracker.cpp
> +++ b/language/backgroundparser/documentchangetracker.cpp
> @@ -237,6 +237,11 @@ KDevelop::RangeInRevision DocumentChangeTracker::transformBetweenRevisions(KDeve
> {
> VERIFY_FOREGROUND_LOCKED
>
> + if ( !((fromRevision == -1 || holdingRevision(fromRevision)) && (toRevision == -1 || holdingRevision(toRevision)) ) ) {
> + qWarning() << "invalid transform: from" << fromRevision << "to" << toRevision
> + << "but not both revisions held: [from/to]:" << holdingRevision(fromRevision) << holdingRevision(toRevision);
> + };
> +
> if((fromRevision == -1 || holdingRevision(fromRevision)) && (toRevision == -1 || holdingRevision(toRevision)))
> {
> m_moving->transformCursor(range.start.line, range.start.column, KTextEditor::MovingCursor::MoveOnInsert, fromRevision, toRevision);
> @@ -348,6 +353,8 @@ void DocumentChangeTracker::unlockRevision(qint64 revision)
> m_moving->unlockRevision(revision);
> m_revisionLocks.erase(it);
> }
> +
> + qDebug() << "** clearing revision" << revision;
> }
>
> qint64 RevisionLockerAndClearer::revision() const
>
> You get _lots_ of those warnings iff you manage to trigger the issue. The best bet seems to be open a relatively large cpp file, and keep removing text and readding it with Ctrl+Z with varying wait times in between (500ms-parse-delay-ish). Do that for a minute or two and it will trigger eventually.
>
> You still get a few of the warning messages sometimes even with the patch applied (but far more without). I think they are from the problem reporter plugin, and I'm not sure if the plugin does something wrong or the parse job.
>
>
> Thanks,
>
> Sven Brauch
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20160721/f06ef297/attachment-0001.html>
More information about the KDevelop-devel
mailing list