git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Martin Ågren <martin.agren@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Lars Schneider <larsxschneider@gmail.com>,
	Git Users <git@vger.kernel.org>
Subject: Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()
Date: Tue, 29 Aug 2017 21:22:08 +0200
Message-ID: <CAN0heSomjqiSStq3eqGgCe21b0Pr0gbAQLL-2EEB6Zfnjj+Mjg@mail.gmail.com> (raw)
In-Reply-To: <20170829185341.s3xlsx4uym7lcluc@sigill.intra.peff.net>

On 29 August 2017 at 20:53, Jeff King <peff@peff.net> wrote:
> On Tue, Aug 29, 2017 at 06:51:52PM +0100, Lars Schneider wrote:
>> What if we run a few selected tests with valgrind and count all files that
>> valgrind mentions (a single leak has multiple file mentions because of
>> the stack trace and other leak indicators). We record these counts and let
>> TravisCI scream if one of the numbers increases.
>>
>> I wonder how stable/fragile such a metric would be as a simple refactoring
>> could easily change these numbers. Below I ran valgrind on t5510 before and
>> after Martin's patch. The diff below clearly shows the pkt-line leak.
>>
>> Would it make sense to pursue something like this in TravisCI to avoid
>> "pkt-line" kind of leaks in the future?
>
> I don't think that would work, because simply adding new tests would
> bump the leak count, without the code actually growing worse.
>
> But think about your strategy for a moment: what you're really trying to
> do is say "these existing leaks are OK because they are too many for us
> to count, but we want to make sure we don't add _new_ ones". We already
> have two techniques for distinguishing old ones from new ones:
>
>   1. Mark existing ones with valgrind suppressions so they do not warn
>      at all.
>
>   2. Fix them, so that the "existing" count drops to zero.
>
> Option (2), of course, has the added side effect that it's actually
> fixing potential problems. :)

One idea would be to report that "t1234 used to complete without leaks,
but now there is a leak". Of course, the leak could be triggered by some
"irrelevant" setup step that was just added and not by the functionality
that is actually tested. But maybe then the reaction might be "oh, let's
fix this one leak so that this test is leak-free again". If that then
also reduces the leakiness of some other tests, good. That might help
against the "there are too many leaks, where should I start?"-effect.

Or similarly "This stack-trace was reported. I haven't seen it before."
It could be a refactoring, it could be an old leak that hasn't been
triggered in that way before, or it could be a new leak.

But to be honest, I think that only works -- in a psychological sense --
once the number of leaks is small enough, however much that is.

One take-away for me from this thread is that memory-leak-plugging seems
to be appreciated, i.e., it's not just an academic problem. I think I'll
look into it some more, i.e., slowly pursue option 2. :-) That might help
give some insights on how to identify interesting leaks.

Martin

  parent reply index

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-27  7:37 Martin Ågren
2017-08-27 15:41 ` Jeff King
2017-08-27 18:25   ` Jeff King
2017-08-27 18:21 ` Lars Schneider
2017-08-27 19:09   ` Martin Ågren
2017-08-27 19:15     ` Jeff King
2017-08-27 20:04     ` Lars Schneider
2017-08-27 23:23       ` Jeff King
2017-08-28  4:11         ` Martin Ågren
2017-08-28 17:52           ` Stefan Beller
2017-08-28 17:58             ` Jeff King
2017-09-05 10:03               ` Junio C Hamano
2017-08-29 17:51           ` Lars Schneider
2017-08-29 18:53             ` Jeff King
2017-08-29 18:58               ` [PATCH] config: use a static lock_file struct Jeff King
2017-08-29 19:09                 ` Brandon Williams
2017-08-29 19:10                   ` Brandon Williams
2017-08-29 19:12                   ` Jeff King
2017-08-30  3:25                     ` Michael Haggerty
2017-08-30  4:31                       ` Jeff King
2017-08-30  4:55                         ` Michael Haggerty
2017-08-30  4:55                         ` Jeff King
2017-08-30  5:55                           ` Jeff King
2017-08-30  7:07                             ` Michael Haggerty
2017-09-02  8:44                               ` Jeff King
2017-09-02 13:50                                 ` Jeff King
2017-08-30  6:55                           ` Michael Haggerty
2017-08-30 19:53                             ` Jeff King
2017-08-30 19:57                               ` Brandon Williams
2017-08-30 20:11                                 ` Jeff King
2017-08-30 21:06                                   ` Brandon Williams
2017-08-31  4:09                                     ` Jeff King
2017-09-06  3:59                 ` Junio C Hamano
2017-09-06 12:41                   ` Jeff King
2017-08-29 19:22               ` Martin Ågren [this message]
2017-08-29 21:48                 ` [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1() Jeff King
2017-08-30  5:31                   ` Jeff King
2017-09-05 10:03                     ` Junio C Hamano
2017-10-10  4:06                       ` [PATCH 0/2] Do not call cmd_*() as a subroutine Junio C Hamano
2017-10-10  4:06                         ` [PATCH 1/2] describe: do not use " Junio C Hamano
2017-10-10 13:43                           ` SZEDER Gábor
2017-10-11  6:00                             ` Junio C Hamano
2017-10-10  4:06                         ` [PATCH 2/2] merge-ours: " Junio C Hamano

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAN0heSomjqiSStq3eqGgCe21b0Pr0gbAQLL-2EEB6Zfnjj+Mjg@mail.gmail.com \
    --to=martin.agren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=larsxschneider@gmail.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox