Opened 6 years ago

Closed 5 years ago

#4264 closed enhancement (wontfix)

Consider dropping pfree calls in liblwgeom

Reported by: Algunenano Owned by: Algunenano
Priority: medium Milestone: PostGIS 3.0.0
Component: postgis Version: master
Keywords: Cc:

Description

Recently I became aware that Postgresql deletes the memory context per tuple, not per function. With that in mind, it doesn't make sense for the library to be calling pfree since waiting it's going to wait until the memory context is freed anyway.

Rough Patch:

diff --git a/libpgcommon/lwgeom_pg.c b/libpgcommon/lwgeom_pg.c
index 1ee41f0cb..2733d3cbe 100644
--- a/libpgcommon/lwgeom_pg.c
+++ b/libpgcommon/lwgeom_pg.c
@@ -113,7 +113,7 @@ pg_realloc(void *mem, size_t size)
 static void
 pg_free(void *ptr)
 {
-  pfree(ptr);
+  //  pfree(ptr);
 }

I decided to test this patch to generate polygon MVTs, and I see a 5-10% improvement in the slowest cases (the ones that do the whole process).

3 main thoughts:

  • Check that there isn't any function is requesting a lot of memory. This might mean redesigning some functions (none come to mind) to avoid allocating in a loop or recursively, but that by itself is a win.
  • Check that it's only applied to liblwgeom when working as a library, and not to external libraries (GEOS, PROJ, etc.): I think this is already true, but I have to make sure.
  • I don't intend to apply this to the extensions code itself (yet).

Any comments are very welcome.

Attachments (1)

20181128_postgis_def_vs_20181128_postgis_nofree_def.pdf (58.5 KB ) - added by Algunenano 6 years ago.
MVT polygon perf comparison (trunk head vs trunk without free calls)

Download all attachments as: .zip

Change History (4)

by Algunenano, 6 years ago

MVT polygon perf comparison (trunk head vs trunk without free calls)

comment:1 by komzpa, 6 years ago

ST_ClusterKMeans is a thing to check (did it allocate in a loop?), also ST_Subdivide may requre many intermediates. Can we get a version of pg_free that just logs free() calls into some array and then frees say on 1000th free?

comment:2 by Algunenano, 6 years ago

ST_ClusterKMeans is a thing to check (did it allocate in a loop?), also ST_Subdivide may requre many intermediates.

An easy way out would be to change handlers in those functions (they are both changing context too) and have pfree acting as it's acting now.

Can we get a version of pg_free that just logs free() calls into some array and then frees say on 1000th free?

I don't think that would be positive, it is the same as giving control to Postgres with the added complexity of handing an intermediate step ourselves.

comment:3 by robe, 5 years ago

Resolution: wontfix
Status: assignedclosed
Note: See TracTickets for help on using tickets.