Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increased performance and buffer preallocation for Writer and WKTWriter #5

Closed

Conversation

meastp
Copy link

@meastp meastp commented Feb 21, 2013

This patch adds the ability to preallocate the buffer of Writer, and also removes an unnecessary intermediate string object in WKTWriter, resulting in increased performance. A test case for separate Writer objects have been added to the test suite. All tests pass successfully.

From the mailing list:

Fra: geos-devel-bounces@lists.osgeo.org [mailto:geos-devel-
bounces@lists.osgeo.org] På vegne av Mats Taraldsvik
Sendt: 18. januar 2013 12:53
Til: geos-devel@lists.osgeo.org
Emne: [geos-devel] Ability to preallocate the WKT stringstream
buffer for really large geometries?

Hi,

Generally, I'm very satisfied with the WKTWriter, which is really
easy to use, and converts the Geometries to the WKT format. However,
when converting large geometries (in this case, contour lines), I
get a major decrease in performance.

By looking at the source code (and profiling), I believe the use of
a stringstream and hence a dynamically increasing string buffer,
constantly reallocating, is causing the performance penalty.
Possibly also copying the large string.

Is there anything I can do to improve this scenario? Do you accept
contributions to fix this (e.g. passing in the expected size of the
resulting WKT string)?

Regards

On Fri, Feb 08, 2013 at 07:22:21AM +0000, Mats Taraldsvik wrote:

Have you considered this issue? Do you accept patches to solve this by
changing the interface (considering GEOS is a port of JTS and follows
it closely)? :)

I'll be glad to review and apply any patch speeding up WKT writing.
I'm fine with providing an enhanced interface for 3.4.x series, as long as the old one remains for backward API compatibility.

If you start with that please consider taking a look at
http://trac.osgeo.org/geos/ticket/355 and
http://trac.osgeo.org/geos/ticket/310

Please also make sure the testsuite covers enough cases.

Thank you !

--strk;

Regards

Mats Taraldsvik

Added constructor in Writer to preallocate the string buffer.
Added an additional constructor to geos::io::Writer that preallocates
the string buffer. By preallocating the string, one can avoid
reallocations of the internal Writer string for large geometries.
Modified the WKTWriter::appendCoordinate method to not create an
intermediate std::string, but instead write directly into the Writer
object. The result is a small performance improvement.
@strk
Copy link
Member

strk commented Feb 21, 2013

I've committed the appendCoordinate optimization in both 3.3 branch and trunk.
As for the Writer changes, can you please start a new testcase for it rather than reusing WKTWriter testcase ?
Also please don't mix formatting and functional changes.

I'm closing this, expecting next pull. Thanks !

@strk strk closed this Feb 21, 2013
sloriot pushed a commit to sloriot/libgeos that referenced this pull request Mar 15, 2018
complete area, minkowskiSum
Algunenano pushed a commit to Algunenano/geos that referenced this pull request Sep 28, 2018
Suite: buildarea
  Test: buildarea1 ...Uninitialized bytes in __interceptor_memcmp at offset 0 inside [0x7fffae24fa80, 4)
==19345==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7f5ed6768c5f in std::char_traits<char>::compare(char const*, char const*, unsigned long) /build/gcc/src/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/char_traits.h:310:25
    libgeos#1 0x7f5ed6768c5f in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::compare(char const*) const /build/gcc/src/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:1424:37
    libgeos#2 0x7f5ed6892f40 in bool std::operator==<char, std::char_traits<char>, std::allocator<char> >(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char const*) /usr/include/c++/8.2.1/bits/basic_string.h:6075:35
    libgeos#3 0x7f5ed6892f40 in geos::geom::GeometryFactory::buildGeometry(std::vector<geos::geom::Geometry*, std::allocator<geos::geom::Geometry*> >*) const /usr/src/debug/geos/src/geom/GeometryFactory.cpp:664:16
    libgeos#4 0x7f5ed690dd48 in geos::operation::overlay::OverlayOp::computeOverlay(geos::operation::overlay::OverlayOp::OpCode) /usr/src/debug/geos/src/operation/overlay/OverlayOp.cpp:839:28
    libgeos#5 0x7f5ed690dee1 in geos::operation::overlay::OverlayOp::getResultGeometry(geos::operation::overlay::OverlayOp::OpCode) /usr/src/debug/geos/src/operation/overlay/OverlayOp.cpp:187:16
    libgeos#6 0x7f5ed690e281 in geos::operation::overlay::OverlayOp::overlayOp(geos::geom::Geometry const*, geos::geom::Geometry const*, geos::operation::overlay::OverlayOp::OpCode) /usr/src/debug/geos/src/operation/overlay/OverlayOp.cpp:93:30
    libgeos#7 0x7f5ed688d11f in geos::operation::overlay::overlayOp::operator()(geos::geom::Geometry const*, geos::geom::Geometry const*) /usr/src/debug/geos/src/geom/../../include/geos/operation/overlay/OverlayOp.h:388:44
    libgeos#8 0x7f5ed688d11f in std::unique_ptr<geos::geom::Geometry, std::default_delete<geos::geom::Geometry> > geos::geom::BinaryOp<geos::operation::overlay::overlayOp>(geos::geom::Geometry const*, geos::geom::Geometry const*, geos::operation::overlay::overlayOp) /usr/src/debug/geos/src/geom/../../include/geos/geom/BinaryOp.h:357:3
    libgeos#9 0x7f5ed688b16f in geos::geom::Geometry::Union(geos::geom::Geometry const*) const /usr/src/debug/geos/src/geom/Geometry.cpp:586:17
    libgeos#10 0x7f5ed691885d in geos::operation::geounion::CascadedPolygonUnion::unionActual(geos::geom::Geometry*, geos::geom::Geometry*) /usr/src/debug/geos/src/operation/union/CascadedPolygonUnion.cpp:370:36
    libgeos#11 0x7f5ed6919080 in geos::operation::geounion::CascadedPolygonUnion::unionOptimized(geos::geom::Geometry*, geos::geom::Geometry*) /usr/src/debug/geos/src/operation/union/CascadedPolygonUnion.cpp:236:27
    libgeos#12 0x7f5ed6919252 in geos::operation::geounion::CascadedPolygonUnion::unionTree(geos::index::strtree::ItemsList*) /usr/src/debug/geos/src/operation/union/CascadedPolygonUnion.cpp:162:23
    libgeos#13 0x7f5ed6919630 in geos::operation::geounion::CascadedPolygonUnion::Union() /usr/src/debug/geos/src/operation/union/CascadedPolygonUnion.cpp:151:21
    libgeos#14 0x7f5ed691983d in geos::operation::geounion::CascadedPolygonUnion::Union(geos::geom::MultiPolygon const*) /usr/src/debug/geos/src/operation/union/CascadedPolygonUnion.cpp:124:20
    libgeos#15 0x7f5ed71db6a8 in GEOSUnionCascaded_r /usr/src/debug/geos/capi/geos_ts_c.cpp:2497:43
    libgeos#16 0x7f5ed7332ed6 in LWGEOM_GEOS_buildArea /home/raul/dev/public/postgis/liblwgeom/lwgeom_geos.c:1124:8
    libgeos#17 0x7f5ed7333164 in lwgeom_buildarea /home/raul/dev/public/postgis/liblwgeom/lwgeom_geos.c:1155:7
    libgeos#18 0x55b755e00bfa in buildarea1 /un/dev_public/postgis/liblwgeom/cunit/cu_buildarea.c:66:9
    libgeos#19 0x7f5ed6fb1117  (/usr/lib/libcunit.so.1+0x4117)
    libgeos#20 0x7f5ed6fb13b1  (/usr/lib/libcunit.so.1+0x43b1)
    libgeos#21 0x7f5ed6fb17b6 in CU_run_all_tests (/usr/lib/libcunit.so.1+0x47b6)
    libgeos#22 0x55b755e7f6b7 in main /un/dev_public/postgis/liblwgeom/cunit/cu_tester.c:177:13
    libgeos#23 0x7f5ed6c3c222 in __libc_start_main (/usr/lib/libc.so.6+0x24222)
    libgeos#24 0x55b755d7f0bd in _start (/un/dev_public/postgis/liblwgeom/cunit/.libs/lt-cu_tester+0x250bd)

SUMMARY: MemorySanitizer: use-of-uninitialized-value /build/gcc/src/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/char_traits.h:310:25 in std::char_traits<char>::compare(char const*, char const*, unsigned long)
strk pushed a commit that referenced this pull request Nov 20, 2018
Suite: buildarea
  Test: buildarea1 ...Uninitialized bytes in __interceptor_memcmp at offset 0 inside [0x7fffae24fa80, 4)
==19345==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7f5ed6768c5f in std::char_traits<char>::compare(char const*, char const*, unsigned long) /build/gcc/src/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/char_traits.h:310:25
    #1 0x7f5ed6768c5f in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::compare(char const*) const /build/gcc/src/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:1424:37
    #2 0x7f5ed6892f40 in bool std::operator==<char, std::char_traits<char>, std::allocator<char> >(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char const*) /usr/include/c++/8.2.1/bits/basic_string.h:6075:35
    #3 0x7f5ed6892f40 in geos::geom::GeometryFactory::buildGeometry(std::vector<geos::geom::Geometry*, std::allocator<geos::geom::Geometry*> >*) const /usr/src/debug/geos/src/geom/GeometryFactory.cpp:664:16
    #4 0x7f5ed690dd48 in geos::operation::overlay::OverlayOp::computeOverlay(geos::operation::overlay::OverlayOp::OpCode) /usr/src/debug/geos/src/operation/overlay/OverlayOp.cpp:839:28
    #5 0x7f5ed690dee1 in geos::operation::overlay::OverlayOp::getResultGeometry(geos::operation::overlay::OverlayOp::OpCode) /usr/src/debug/geos/src/operation/overlay/OverlayOp.cpp:187:16
    #6 0x7f5ed690e281 in geos::operation::overlay::OverlayOp::overlayOp(geos::geom::Geometry const*, geos::geom::Geometry const*, geos::operation::overlay::OverlayOp::OpCode) /usr/src/debug/geos/src/operation/overlay/OverlayOp.cpp:93:30
    #7 0x7f5ed688d11f in geos::operation::overlay::overlayOp::operator()(geos::geom::Geometry const*, geos::geom::Geometry const*) /usr/src/debug/geos/src/geom/../../include/geos/operation/overlay/OverlayOp.h:388:44
    #8 0x7f5ed688d11f in std::unique_ptr<geos::geom::Geometry, std::default_delete<geos::geom::Geometry> > geos::geom::BinaryOp<geos::operation::overlay::overlayOp>(geos::geom::Geometry const*, geos::geom::Geometry const*, geos::operation::overlay::overlayOp) /usr/src/debug/geos/src/geom/../../include/geos/geom/BinaryOp.h:357:3
    #9 0x7f5ed688b16f in geos::geom::Geometry::Union(geos::geom::Geometry const*) const /usr/src/debug/geos/src/geom/Geometry.cpp:586:17
    #10 0x7f5ed691885d in geos::operation::geounion::CascadedPolygonUnion::unionActual(geos::geom::Geometry*, geos::geom::Geometry*) /usr/src/debug/geos/src/operation/union/CascadedPolygonUnion.cpp:370:36
    #11 0x7f5ed6919080 in geos::operation::geounion::CascadedPolygonUnion::unionOptimized(geos::geom::Geometry*, geos::geom::Geometry*) /usr/src/debug/geos/src/operation/union/CascadedPolygonUnion.cpp:236:27
    #12 0x7f5ed6919252 in geos::operation::geounion::CascadedPolygonUnion::unionTree(geos::index::strtree::ItemsList*) /usr/src/debug/geos/src/operation/union/CascadedPolygonUnion.cpp:162:23
    #13 0x7f5ed6919630 in geos::operation::geounion::CascadedPolygonUnion::Union() /usr/src/debug/geos/src/operation/union/CascadedPolygonUnion.cpp:151:21
    #14 0x7f5ed691983d in geos::operation::geounion::CascadedPolygonUnion::Union(geos::geom::MultiPolygon const*) /usr/src/debug/geos/src/operation/union/CascadedPolygonUnion.cpp:124:20
    #15 0x7f5ed71db6a8 in GEOSUnionCascaded_r /usr/src/debug/geos/capi/geos_ts_c.cpp:2497:43
    #16 0x7f5ed7332ed6 in LWGEOM_GEOS_buildArea /home/raul/dev/public/postgis/liblwgeom/lwgeom_geos.c:1124:8
    #17 0x7f5ed7333164 in lwgeom_buildarea /home/raul/dev/public/postgis/liblwgeom/lwgeom_geos.c:1155:7
    #18 0x55b755e00bfa in buildarea1 /un/dev_public/postgis/liblwgeom/cunit/cu_buildarea.c:66:9
    #19 0x7f5ed6fb1117  (/usr/lib/libcunit.so.1+0x4117)
    #20 0x7f5ed6fb13b1  (/usr/lib/libcunit.so.1+0x43b1)
    #21 0x7f5ed6fb17b6 in CU_run_all_tests (/usr/lib/libcunit.so.1+0x47b6)
    #22 0x55b755e7f6b7 in main /un/dev_public/postgis/liblwgeom/cunit/cu_tester.c:177:13
    #23 0x7f5ed6c3c222 in __libc_start_main (/usr/lib/libc.so.6+0x24222)
    #24 0x55b755d7f0bd in _start (/un/dev_public/postgis/liblwgeom/cunit/.libs/lt-cu_tester+0x250bd)

SUMMARY: MemorySanitizer: use-of-uninitialized-value /build/gcc/src/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/char_traits.h:310:25 in std::char_traits<char>::compare(char const*, char const*, unsigned long)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants