Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#1102 closed defect (fixed)

Memory leaks in liblwgeom ?

Reported by: strk Owned by: strk
Priority: blocker Milestone: PostGIS 2.0.0
Component: postgis Version: master
Keywords: Cc: robe

Description

Valgrind analisys of liblwgeom/cunit/cu_tester run:

==6914== HEAP SUMMARY:
==6914==     in use at exit: 69,592 bytes in 1,022 blocks
==6914==   total heap usage: 9,990 allocs, 8,968 frees, 772,317 bytes allocated

I'll attach full log.

Attachments (2)

valgrind-r7570.txt (385.0 KB ) - added by strk 13 years ago.
run of liblwgeom/cunit/cu_tester in r7570 with valgrind
lwgeom_clone_ptarray.diff (2.2 KB ) - added by strk 13 years ago.
patch ensuring base type clones do copy POINTARRAY memory (w/out serialized points)

Download all attachments as: .zip

Change History (30)

by strk, 13 years ago

Attachment: valgrind-r7570.txt added

run of liblwgeom/cunit/cu_tester in r7570 with valgrind

comment:1 by strk, 13 years ago

With r7578 I've fixed a few leaks which were in the unit tests themselves, but next big leak really comes from library, and is related to WKT errors during parsing. It is triggered by passing 'TIN(((0 1 2,3 4 5,6 7 8,9 10 11,0 1 2)))' to lwgeom_from_ewkt. The code will allocate a POINTARRAY and then bail out calling lwerror, failing to release the allocated POINARRAY.

comment:2 by strk, 13 years ago

I belive the culprit here is lwgeom_clone taking no effort to properly set the referenced PTARRAY readonly flags, so if you clone an LWGEOM with fully-owned PTARRAY you'd end up with two versions pointing to the same PTARRAY and both marked as owning it (lw*_clone uses memcpy)

comment:3 by strk, 13 years ago

Uhm, it's more complex than that. the memcpy doesn't even copy the POINTARRAY but keeps it by reference. We'd need to have a ptarray_clone doing shallow copy and ptarray_clone_deep to do what the current ptarray_clone does… confusing.

comment:4 by strk, 13 years ago

*manually* copying the POINARRAY (but not the serialized_pointlist) seems to resolve the issue. Would still be worth cleaning things up so that ptarray_clone does this instead.

by strk, 13 years ago

Attachment: lwgeom_clone_ptarray.diff added

patch ensuring base type clones do copy POINTARRAY memory (w/out serialized points)

comment:5 by strk, 13 years ago

The attached patch implements proper cloning of POINTARRAY elements in lwpoint_clone, lwline_clone and lwpoly_clone. Serialized pointlist is _not_ copied but simply marked as READONLY. A more integrated approach might be porting the readonly/non-readonly (or better _owned_/_not_owned_ flags in the LWGEOM structure itself, but I'm not sure it'd save us much.

Conceptual and practical reviews of the patch are welcome. If we like the idea, it should be better documented that lwgeom_clone does not copy the _serialized_points_, not the POINTARRAY (as currently documented).

comment:6 by pramsey, 13 years ago

You have shown this patch to affect your TIN case? Because I feel like I've tracked the TIN case to an altogether different piece of code, the roll-back code in the WKT parser which goes into effect when an error is caught. It correctly rolls back objects still on the parser stack, but at the time the error is forced for your case the ptarray has already been moved off the stack so it doesn't get freed.

This is not to say that the semantics of clone() might be wrong/off, they just don't seem germane to the original ticket issue.

comment:7 by pramsey, 13 years ago

I think I have a fix for the actual ticket issue at r7587

comment:8 by strk, 13 years ago

I confirm my _clone patch is not directly related to the _specific_ issue I was talking about in the first comments. Your patch is probably good for that. Since this ticket is generally summarized as "memory leaks in liblwgeom" I was tracking a different spot, the one that forced me commit r7579 as a partial rollback of some memory cleanup work done with r7578.

See how it seems impossible to release all memory in the function modified by r7579 unless (if things didn't change with your modifications).

comment:9 by strk, 13 years ago

After r7590 the only leaks detected by valgrind runs of liblwgeom/cunit/cu_tester come from cu_homogenize:

definitely lost: 15,667 bytes in 346 blocks

I think we want these fixed before releasing 2.0

comment:10 by strk, 13 years ago

Oh, another small but continuos leak seems to be in WKT parser, where 2 bytes get struped and are lost forever (for each WKT input parsed). Thi sis lwin_wkt_lex.l:96

comment:11 by strk, 13 years ago

Priority: mediumblocker

comment:12 by pramsey, 13 years ago

I think I have recovered the missing 2 bytes. r7591 I don't think we need to strdup that data because we never use it as a string later on, we just look at it to see if it has Z or M in it.

comment:13 by strk, 13 years ago

Alright, definitely lost down to 15,601 bytes in 314 blocks as of r7591

comment:14 by strk, 13 years ago

This seems to also leak on error:

lwgeom_from_ewkt("POLYHEDRALSURFACE(((0 1 2,3 4 5,6 7,0 1 2)))", PARSER_CHECK_NONE);

It leaks a POINARRAY allocated by wkt_parser_ptarray_new (lwin_wkt.c:259)

comment:15 by strk, 13 years ago

As of r7592, definitely lost bytes are down to 9,441 bytes in 264 blocks

comment:16 by pramsey, 13 years ago

OK, closed that one at r7593

comment:17 by strk, 13 years ago

Cc: robe added

Great, and as of r7600 we have 2,240 bytes in 162 blocks definitely lost.

Robe, next one is for you: lwout_x3d.c:64 creates a temporary lwgeom using lwgeom_as_multi and never releases it.

comment:18 by strk, 13 years ago

I'll do the lwout_x3d.c part, so I also fix the warnings…

comment:19 by strk, 13 years ago

It turns out the lwout_x3d.c part is also related to lwgeom_clone doing a partial clone. See r7602

comment:20 by strk, 13 years ago

Plan: rename ptarray_clone to ptarray_clone_deep and keep ptarray_clone as a shallow copy too (to conform with lwgeom_clone).

comment:21 by strk, 13 years ago

ptarray_clone → ptarray_clone_deep done in r7506, time to refine my _clone patch to use a new ptarray_clone for shallow cloning

comment:22 by strk, 13 years ago

I've added ptarray_clone (shallow) in r7608, togheter with unit test for lwgeom_clone and lwgeom_release round.

In r7609 I've cleaned up lwgeom_homogenize to use lwgeom_free safely.

Definitely lost now: 248 bytes in 8 blocks (x3d and lwcollection_extract)

lwcollection_extract problem seems to be that geometries from input collection are added to output collection w/out cloning. I think it'd be ok to clone, given our clones are shallow by default (no serialized pointlist copied)

comment:23 by strk, 13 years ago

r7610 makes lwcollection_extract shallow clone the extracted components, taking lost bytes down to 48 bytes in 6 blocks

comment:24 by strk, 13 years ago

definitely lost 16 bytes in 2 blocks as of r7611

comment:25 by strk, 13 years ago

Owner: changed from pramsey to strk
Status: newassigned

comment:26 by strk, 13 years ago

Resolution: fixed
Status: assignedclosed

And… definitely lost: 0 bytes in 0 blocks in r7612 That's all, folks.

comment:27 by mcayland, 13 years ago

Nice work strk! :)

comment:28 by pramsey, 13 years ago

Indeed, I feel cleaner already. Thanks strk.

Note: See TracTickets for help on using tickets.