Re: Fix memleak: temporary skyline objects for systems were never deleted (issue 4923048)
On 2011-08-27, at 04:20 , pkx1...@gmail.com wrote: > passes make and reg tests > > http://codereview.appspot.com/4923048/ You could clean up Skyline::distance by pulling lines 532-548 into their own function and letting Skyline::distance call it with different options: if (horizon_padding != 0.0) { Skyline padded_this(. . .); Skyline padded_other(. . .); return unpadded_distance(padded_this, padded_other); } else { return unpadded_distance(this, &other); } -- Dan ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix memleak: temporary skyline objects for systems were never deleted (issue 4923048)
passes make and reg tests http://codereview.appspot.com/4923048/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix memleak: temporary skyline objects for systems were never deleted (issue 4923048)
On Wed, Aug 24, 2011 at 10:57 AM, David Kastrup wrote: > Han-Wen Nienhuys writes: > >>> Skylines are smobs. The usual way to delete them would be to >>> unprotect them once they have been registered by some >>> garbage-collectable object (or a SCM variable that is being used for >>> accessing them). >> >> They are simple smobs, though, so this pattern here is not uncommon. >> You could also try to allocate them on the stack. > > I think it would make me less uncomfortable than using explicit delete. I don't think in the end neither choice matters very much; the stack is also awkward, because the instantiation happens in a if(){}. I would probably write as follows Skyline *withpadding = 0; ... if ( .. ) { withpadding = new Skyline( .. ) } delete withpadding; this still has a leak-risk if someone adds a return in the middle. To fix that, you'd have to use something like Boost's scoped_ptr<>. -- Han-Wen Nienhuys - han...@xs4all.nl - http://www.xs4all.nl/~hanwen ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix memleak: temporary skyline objects for systems were never deleted (issue 4923048)
Am Mittwoch, 24. August 2011, 15:38:21 schrieb Han-Wen Nienhuys: > On Wed, Aug 24, 2011 at 3:38 AM, David Kastrup wrote: > > Skylines are smobs. The usual way to delete them would be to unprotect > > them once they have been registered by some garbage-collectable object > > (or a SCM variable that is being used for accessing them). > > They are simple smobs, though, so this pattern here is not uncommon. > You could also try to allocate them on the stack. So, what do you think would be the appropriate fix for this memleak? I don't see how I could allocate the skylines on the stack just for the case of systems (notice that the "new Skyline" allocations are inside an if!). We don't want to allocate a new skyline for all Skyline::distance() calls, just for those, where the padding has not been added yet. OTOH, I don't know anything about how guile is able to handle C++ class instances and how to tell it to delete a class instance allocated with new. However, currently valgrind says the skyline memory is definitely lost: ==28530== 2,040 (24 direct, 2,016 indirect) bytes in 2 blocks are definitely lost in loss record 6,104 of 6,263 ==28530==at 0x402641D: operator new(unsigned int) (vg_replace_malloc.c:255) ==28530==by 0x8234A28: Skyline::distance(Skyline const&, double) const (skyline.cc:526) ==28530==by 0x81BCC8F: Page_layout_problem::append_system(System*, Spring const&, double, double) (page-layout-problem.cc:496) ==28530==by 0x81BC37D: Page_layout_problem::Page_layout_problem(Paper_book*, scm_unused_struct*, scm_unused_struct*, int) (page-layout-problem.cc:411) ==28530==by 0x81ABCD8: Page_breaking::get_page_configuration(scm_unused_struct*, int, int, bool, bool) (page-breaking.cc:537) ==28530==by 0x81AC2B0: Page_breaking::make_pages(std::vector >, scm_unused_struct*) (page-breaking.cc:617) ==28530==by 0x81A3891: Optimal_page_breaking::solve() (optimal-page- breaking.cc:211) ==28530==by 0x81B9ADE: ly_optimal_breaking(scm_unused_struct*) (page- breaking-scheme.cc:42) ==28530==by 0x4090F18: scm_dapply (in /usr/lib/libguile.so.17.3.1) Cheers, Reinhold -- -- Reinhold Kainhofer, reinh...@kainhofer.com, http://reinhold.kainhofer.com/ * Financial & Actuarial Math., Vienna Univ. of Technology, Austria * http://www.fam.tuwien.ac.at/, DVR: 0005886 * LilyPond, Music typesetting, http://www.lilypond.org ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix memleak: temporary skyline objects for systems were never deleted (issue 4923048)
Han-Wen Nienhuys writes: >> Skylines are smobs. The usual way to delete them would be to >> unprotect them once they have been registered by some >> garbage-collectable object (or a SCM variable that is being used for >> accessing them). > > They are simple smobs, though, so this pattern here is not uncommon. > You could also try to allocate them on the stack. I think it would make me less uncomfortable than using explicit delete. -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix memleak: temporary skyline objects for systems were never deleted (issue 4923048)
On Wed, Aug 24, 2011 at 3:38 AM, David Kastrup wrote: > reinhold.kainho...@gmail.com writes: > >> Reviewers: , >> >> Message: >> This patch fixes a memleak: Some temporary skylines were never >> deleted... >> Please review >> >> Description: >> Fix memleak: temporary skyline objects for systems were never deleted >> >> Please review this at http://codereview.appspot.com/4923048/ >> >> Affected files: >> M lily/skyline.cc >> >> >> Index: lily/skyline.cc >> diff --git a/lily/skyline.cc b/lily/skyline.cc >> index >> b6ea6b791bbbfa09aaa28e91da0a619c33085890..8f62710b0947fa8e48ffcbc7e181cba4709c96de >> 100644 >> --- a/lily/skyline.cc >> +++ b/lily/skyline.cc >> @@ -514,6 +514,7 @@ Skyline::distance (Skyline const &other, Real >> horizon_padding) const >> >> Skyline const *padded_this = this; >> Skyline const *padded_other = &other; >> + bool created_tmp_skylines = false; >> >> /* >> For systems, padding is not added at creation time. Padding is >> @@ -525,6 +526,7 @@ Skyline::distance (Skyline const &other, Real >> horizon_padding) const >> { >> padded_this = new Skyline (*padded_this, horizon_padding, X_AXIS); >> padded_other = new Skyline (*padded_other, horizon_padding, X_AXIS); >> + created_tmp_skylines = true; >> } >> >> list::const_iterator i = padded_this->buildings_.begin (); >> @@ -544,6 +546,13 @@ Skyline::distance (Skyline const &other, Real >> horizon_padding) const >> j++; >> start = end; >> } >> + >> + if (created_tmp_skylines) >> + { >> + delete padded_this; >> + delete padded_other; >> + } >> + >> return dist; >> } > > Skylines are smobs. The usual way to delete them would be to unprotect > them once they have been registered by some garbage-collectable object > (or a SCM variable that is being used for accessing them). They are simple smobs, though, so this pattern here is not uncommon. You could also try to allocate them on the stack. -- Han-Wen Nienhuys - han...@xs4all.nl - http://www.xs4all.nl/~hanwen ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix memleak: temporary skyline objects for systems were never deleted (issue 4923048)
reinhold.kainho...@gmail.com writes: > Reviewers: , > > Message: > This patch fixes a memleak: Some temporary skylines were never > deleted... > Please review > > Description: > Fix memleak: temporary skyline objects for systems were never deleted > > Please review this at http://codereview.appspot.com/4923048/ > > Affected files: >M lily/skyline.cc > > > Index: lily/skyline.cc > diff --git a/lily/skyline.cc b/lily/skyline.cc > index > b6ea6b791bbbfa09aaa28e91da0a619c33085890..8f62710b0947fa8e48ffcbc7e181cba4709c96de > > 100644 > --- a/lily/skyline.cc > +++ b/lily/skyline.cc > @@ -514,6 +514,7 @@ Skyline::distance (Skyline const &other, Real > horizon_padding) const > > Skyline const *padded_this = this; > Skyline const *padded_other = &other; > + bool created_tmp_skylines = false; > > /* > For systems, padding is not added at creation time. Padding is > @@ -525,6 +526,7 @@ Skyline::distance (Skyline const &other, Real > horizon_padding) const > { > padded_this = new Skyline (*padded_this, horizon_padding, X_AXIS); > padded_other = new Skyline (*padded_other, horizon_padding, X_AXIS); > + created_tmp_skylines = true; > } > > list::const_iterator i = padded_this->buildings_.begin (); > @@ -544,6 +546,13 @@ Skyline::distance (Skyline const &other, Real > horizon_padding) const > j++; > start = end; > } > + > + if (created_tmp_skylines) > +{ > + delete padded_this; > + delete padded_other; > +} > + > return dist; > } Skylines are smobs. The usual way to delete them would be to unprotect them once they have been registered by some garbage-collectable object (or a SCM variable that is being used for accessing them). -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Fix memleak: temporary skyline objects for systems were never deleted (issue 4923048)
Reviewers: , Message: This patch fixes a memleak: Some temporary skylines were never deleted... Please review Description: Fix memleak: temporary skyline objects for systems were never deleted Please review this at http://codereview.appspot.com/4923048/ Affected files: M lily/skyline.cc Index: lily/skyline.cc diff --git a/lily/skyline.cc b/lily/skyline.cc index b6ea6b791bbbfa09aaa28e91da0a619c33085890..8f62710b0947fa8e48ffcbc7e181cba4709c96de 100644 --- a/lily/skyline.cc +++ b/lily/skyline.cc @@ -514,6 +514,7 @@ Skyline::distance (Skyline const &other, Real horizon_padding) const Skyline const *padded_this = this; Skyline const *padded_other = &other; + bool created_tmp_skylines = false; /* For systems, padding is not added at creation time. Padding is @@ -525,6 +526,7 @@ Skyline::distance (Skyline const &other, Real horizon_padding) const { padded_this = new Skyline (*padded_this, horizon_padding, X_AXIS); padded_other = new Skyline (*padded_other, horizon_padding, X_AXIS); + created_tmp_skylines = true; } list::const_iterator i = padded_this->buildings_.begin (); @@ -544,6 +546,13 @@ Skyline::distance (Skyline const &other, Real horizon_padding) const j++; start = end; } + + if (created_tmp_skylines) +{ + delete padded_this; + delete padded_other; +} + return dist; } ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel