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 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)
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;
}
listBuilding::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
Re: Fix memleak: temporary skyline objects for systems were never deleted (issue 4923048)
On Wed, Aug 24, 2011 at 3:38 AM, David Kastrup d...@gnu.org 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;
}
listBuilding::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)
Han-Wen Nienhuys hanw...@gmail.com 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)
Am Mittwoch, 24. August 2011, 15:38:21 schrieb Han-Wen Nienhuys: On Wed, Aug 24, 2011 at 3:38 AM, David Kastrup d...@gnu.org 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::vectorunsigned int, std::allocatorunsigned int , 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)
On Wed, Aug 24, 2011 at 10:57 AM, David Kastrup d...@gnu.org wrote:
Han-Wen Nienhuys hanw...@gmail.com 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
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;
}
listBuilding::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
