Arnaud A. de Grandmaison
2014-Sep-10 22:19 UTC
[LLVMdev] Leaks in PBQPBuilderWithCoalescing::build ?
Hi Lang, In PBQPBuilderWithCoalescing::build, around line 360, we have code looking like: … PBQP::Vector newCosts(g.getNodeCosts(node)); addPhysRegCoalesce(newCosts, pregOpt, cBenefit); g.setNodeCosts(node, newCosts); … I suspect the leak occurs around the setNodeCosts method, and I have trouble understanding how it handles the case where the node already has costs. It seems to me that: - we make of copy of the node’s costs (probably because someone else can refer to it ?) - we modify the copy - we set the node’s new costs. But what is supposed to happen there, especially in the CostAllocator part ? Could you shed some light there ? Thanks, Arnaud From: Lang Hames [mailto:lhames at gmail.com] Sent: 10 September 2014 19:26 To: Arnaud De Grandmaison Subject: Re: Leaks in PBQPBuilderWithCoalescing::build ? Thanks Arnaud! - Lang. On Wed, Sep 10, 2014 at 11:23 AM, Arnaud A. de Grandmaison <arnaud.degrandmaison at arm.com> wrote: Hi Lang, For your information, the leak sanitizer found something when I committed my patch: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/4506/steps/check-llvm%20asan/logs/stdio I will try to have a look at it. Cheers, -- Arnaud A. de Grandmaison -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140910/239f0d00/attachment.html>
Hi Arnaud, You're right: newCosts should be a copy of the node's costs, as it may be modified. When we call setNodeCost's, the PBQP::Vector's assignment operator should free the old costs, but I may have misimplemented that (or forgotten it entirely). Cheers, Lang. On Wed, Sep 10, 2014 at 3:19 PM, Arnaud A. de Grandmaison < arnaud.degrandmaison at arm.com> wrote:> Hi Lang, > > > > In PBQPBuilderWithCoalescing::build, around line 360, we have code looking > like: > > > > … > > PBQP::Vector newCosts(g.getNodeCosts(node)); > > addPhysRegCoalesce(newCosts, pregOpt, cBenefit); > > g.setNodeCosts(node, newCosts); > > … > > > > I suspect the leak occurs around the setNodeCosts method, and I have > trouble understanding how it handles the case where the node already has > costs. > > > > It seems to me that: > > - we make of copy of the node’s costs (probably because someone else can > refer to it ?) > > - we modify the copy > > - we set the node’s new costs. But what is supposed to happen there, > especially in the CostAllocator part ? > > > > Could you shed some light there ? > > > > Thanks, > > Arnaud > > > > > > > > *From:* Lang Hames [mailto:lhames at gmail.com] > *Sent:* 10 September 2014 19:26 > *To:* Arnaud De Grandmaison > *Subject:* Re: Leaks in PBQPBuilderWithCoalescing::build ? > > > > Thanks Arnaud! > > > > - Lang. > > > > On Wed, Sep 10, 2014 at 11:23 AM, Arnaud A. de Grandmaison < > arnaud.degrandmaison at arm.com> wrote: > > Hi Lang, > > > > For your information, the leak sanitizer found something when I committed > my patch: > > > > > http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/4506/steps/check-llvm%20asan/logs/stdio > > > > I will try to have a look at it. > > > > Cheers, > > -- > > Arnaud A. de Grandmaison > > > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140910/e7bccebf/attachment.html>
Hi Arnaud, Out of interest - what makes you suspect the vector cost changes, rather than the matrix ones? Is the tool indicating a leak due to those lines? The memory operations for the vector costs should be fairly straightforward, but the matrix operations use a value-pool system behind the scenes to reduce the PBQP allocator's memory overhead. The extra complexity of that makes it a prime suspect for memory leaks. Cheers, Lang. On Wed, Sep 10, 2014 at 4:03 PM, Lang Hames <lhames at gmail.com> wrote:> Hi Arnaud, > > You're right: newCosts should be a copy of the node's costs, as it may be > modified. When we call setNodeCost's, the PBQP::Vector's assignment > operator should free the old costs, but I may have misimplemented that (or > forgotten it entirely). > > Cheers, > Lang. > > > On Wed, Sep 10, 2014 at 3:19 PM, Arnaud A. de Grandmaison < > arnaud.degrandmaison at arm.com> wrote: > >> Hi Lang, >> >> >> >> In PBQPBuilderWithCoalescing::build, around line 360, we have code >> looking like: >> >> >> >> … >> >> PBQP::Vector newCosts(g.getNodeCosts(node)); >> >> addPhysRegCoalesce(newCosts, pregOpt, cBenefit); >> >> g.setNodeCosts(node, newCosts); >> >> … >> >> >> >> I suspect the leak occurs around the setNodeCosts method, and I have >> trouble understanding how it handles the case where the node already has >> costs. >> >> >> >> It seems to me that: >> >> - we make of copy of the node’s costs (probably because someone else can >> refer to it ?) >> >> - we modify the copy >> >> - we set the node’s new costs. But what is supposed to happen there, >> especially in the CostAllocator part ? >> >> >> >> Could you shed some light there ? >> >> >> >> Thanks, >> >> Arnaud >> >> >> >> >> >> >> >> *From:* Lang Hames [mailto:lhames at gmail.com] >> *Sent:* 10 September 2014 19:26 >> *To:* Arnaud De Grandmaison >> *Subject:* Re: Leaks in PBQPBuilderWithCoalescing::build ? >> >> >> >> Thanks Arnaud! >> >> >> >> - Lang. >> >> >> >> On Wed, Sep 10, 2014 at 11:23 AM, Arnaud A. de Grandmaison < >> arnaud.degrandmaison at arm.com> wrote: >> >> Hi Lang, >> >> >> >> For your information, the leak sanitizer found something when I committed >> my patch: >> >> >> >> >> http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/4506/steps/check-llvm%20asan/logs/stdio >> >> >> >> I will try to have a look at it. >> >> >> >> Cheers, >> >> -- >> >> Arnaud A. de Grandmaison >> >> >> >> >> > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140910/77b8afd7/attachment.html>
While I'm not sure where the leak is, using some pre-canned memory management might help... Attached is a patch that changes this allocation to use shared_ptr, perhaps it'll address the bug? (ideally we shouldn't need the intrusive ref counting (std::enable_shared_from_this) but instead have a weak_set that has std::weak_ptr in it & implicitly removes elements as they become null (probably on a harvesting schedule, rather than with a direct callback as is currently implemented)) pbqp_leak.diff <https://docs.google.com/file/d/0B0jpkch3iC_7TXFVU2hCcUpfZXM/edit?usp=drive_web> On Wed, Sep 10, 2014 at 3:19 PM, Arnaud A. de Grandmaison < arnaud.degrandmaison at arm.com> wrote:> Hi Lang, > > > > In PBQPBuilderWithCoalescing::build, around line 360, we have code looking > like: > > > > … > > PBQP::Vector newCosts(g.getNodeCosts(node)); > > addPhysRegCoalesce(newCosts, pregOpt, cBenefit); > > g.setNodeCosts(node, newCosts); > > … > > > > I suspect the leak occurs around the setNodeCosts method, and I have > trouble understanding how it handles the case where the node already has > costs. > > > > It seems to me that: > > - we make of copy of the node’s costs (probably because someone else can > refer to it ?) > > - we modify the copy > > - we set the node’s new costs. But what is supposed to happen there, > especially in the CostAllocator part ? > > > > Could you shed some light there ? > > > > Thanks, > > Arnaud > > > > > > > > *From:* Lang Hames [mailto:lhames at gmail.com] > *Sent:* 10 September 2014 19:26 > *To:* Arnaud De Grandmaison > *Subject:* Re: Leaks in PBQPBuilderWithCoalescing::build ? > > > > Thanks Arnaud! > > > > - Lang. > > > > On Wed, Sep 10, 2014 at 11:23 AM, Arnaud A. de Grandmaison < > arnaud.degrandmaison at arm.com> wrote: > > Hi Lang, > > > > For your information, the leak sanitizer found something when I committed > my patch: > > > > > http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/4506/steps/check-llvm%20asan/logs/stdio > > > > I will try to have a look at it. > > > > Cheers, > > -- > > Arnaud A. de Grandmaison > > > > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140910/c64828b3/attachment.html>
Oooh. Neat. Thanks Dave. Please go ahead and commit that. Arnaud - I have no idea whether Dave's patch will help with this bug, but it's certainly worth testing. - Lang. On Wed, Sep 10, 2014 at 4:10 PM, David Blaikie <dblaikie at gmail.com> wrote:> While I'm not sure where the leak is, using some pre-canned memory > management might help... > > Attached is a patch that changes this allocation to use shared_ptr, > perhaps it'll address the bug? > > (ideally we shouldn't need the intrusive ref counting > (std::enable_shared_from_this) but instead have a weak_set that has > std::weak_ptr in it & implicitly removes elements as they become null > (probably on a harvesting schedule, rather than with a direct callback as > is currently implemented)) > pbqp_leak.diff > <https://docs.google.com/file/d/0B0jpkch3iC_7TXFVU2hCcUpfZXM/edit?usp=drive_web> > > > On Wed, Sep 10, 2014 at 3:19 PM, Arnaud A. de Grandmaison < > arnaud.degrandmaison at arm.com> wrote: > >> Hi Lang, >> >> >> >> In PBQPBuilderWithCoalescing::build, around line 360, we have code >> looking like: >> >> >> >> … >> >> PBQP::Vector newCosts(g.getNodeCosts(node)); >> >> addPhysRegCoalesce(newCosts, pregOpt, cBenefit); >> >> g.setNodeCosts(node, newCosts); >> >> … >> >> >> >> I suspect the leak occurs around the setNodeCosts method, and I have >> trouble understanding how it handles the case where the node already has >> costs. >> >> >> >> It seems to me that: >> >> - we make of copy of the node’s costs (probably because someone else can >> refer to it ?) >> >> - we modify the copy >> >> - we set the node’s new costs. But what is supposed to happen there, >> especially in the CostAllocator part ? >> >> >> >> Could you shed some light there ? >> >> >> >> Thanks, >> >> Arnaud >> >> >> >> >> >> >> >> *From:* Lang Hames [mailto:lhames at gmail.com] >> *Sent:* 10 September 2014 19:26 >> *To:* Arnaud De Grandmaison >> *Subject:* Re: Leaks in PBQPBuilderWithCoalescing::build ? >> >> >> >> Thanks Arnaud! >> >> >> >> - Lang. >> >> >> >> On Wed, Sep 10, 2014 at 11:23 AM, Arnaud A. de Grandmaison < >> arnaud.degrandmaison at arm.com> wrote: >> >> Hi Lang, >> >> >> >> For your information, the leak sanitizer found something when I committed >> my patch: >> >> >> >> >> http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/4506/steps/check-llvm%20asan/logs/stdio >> >> >> >> I will try to have a look at it. >> >> >> >> Cheers, >> >> -- >> >> Arnaud A. de Grandmaison >> >> >> >> >> >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140910/568877da/attachment.html>