go-spatial / geom

Geometry interfaces to help drive interoperability within the Go geospatial community
MIT License
168 stars 37 forks source link

wkt: improvements #69

Closed ear7h closed 5 years ago

ear7h commented 5 years ago

performance

This pr has a pretty good performance improvement over the previous encoder, both in cpu time and allocations. The most important function in all of this is formatFloat for an even greater performance improvement it could be written in assembly or maybe use the 'e' formatter.

A simple change that came to mind is also:

type Encoder struct {
   w io.Writer
   buf []byte // set to make([]byte, 0, 16)
}

func (enc *Encoder) formatFloat(f float64) error {
    // for reference, the min value of EPSG:3857 -20026376.390 -> 13 characters
    buf := strconv.AppendFloat(enc.buf, f, 'f', 3, 64)
    i := len(buf) - 1;
    for ; i >= 0; i-- {
        if buf[i] != '0' {
            break
        }
    }
    if buf[i] == '.' {
        i--
    }
    buf = buf[:i+1]
    _, err := enc.w.Write(buf)
    enc.buf = enc.buf[:0]
    return err
}

benchmarks

I'm using the following command for running benchmarks:

go test -v -run=xxx -bench=. --benchtime=5s -benchmem

before

goos: darwin
goarch: amd64
pkg: github.com/go-spatial/geom/encoding/wkt
BenchmarkEncodeSin100-4        88194         67946 ns/op       22624 B/op        614 allocs/op
BenchmarkEncodeSin1000-4        8854        681131 ns/op      214720 B/op       6017 allocs/op
BenchmarkEncodeTile-4              6     852446852 ns/op    597514269 B/op   7658125 allocs/op
PASS
ok      github.com/go-spatial/geom/encoding/wkt 18.828s

after

goos: darwin
goarch: amd64
pkg: github.com/go-spatial/geom/encoding/wkt
BenchmarkEncodeSin100-4       108333         56824 ns/op        9817 B/op        411 allocs/op
BenchmarkEncodeSin1000-4       10000        549257 ns/op       87777 B/op       4014 allocs/op
BenchmarkEncodeTile-4              8     639061782 ns/op    123882434 B/op   5041352 allocs/op
PASS
ok      github.com/go-spatial/geom/encoding/wkt 18.062s

latest

goarch: amd64
pkg: github.com/go-spatial/geom/encoding/wkt
BenchmarkEncodeSin100-4               128605         46471 ns/op        6000 B/op          6 allocs/op
BenchmarkEncodeSin1000-4               12789        471229 ns/op       48496 B/op          8 allocs/op
BenchmarkEncodeTile-4                     10     513830525 ns/op    67322720 B/op        112 allocs/op
BenchmarkEncodeTilePrealloc-4             10     502564612 ns/op    33556064 B/op         99 allocs/op
BenchmarkEncodeTileNoprealloc-4           10     528629372 ns/op    104913264 B/op       120 allocs/op
PASS
ok      github.com/go-spatial/geom/encoding/wkt 34.258s

latest with 'e' parameter

goos: darwin
goarch: amd64
pkg: github.com/go-spatial/geom/encoding/wkt
BenchmarkEncodeSin100-4               302805         19736 ns/op        6000 B/op          6 allocs/op
BenchmarkEncodeSin1000-4               30608        196121 ns/op       89456 B/op          9 allocs/op
BenchmarkEncodeTile-4                     24     219000498 ns/op    67322720 B/op        112 allocs/op
BenchmarkEncodeTilePrealloc-4             28     207262018 ns/op    33556064 B/op         99 allocs/op
BenchmarkEncodeTileNoprealloc-4           26     225557254 ns/op    100664923 B/op       120 allocs/op
PASS
ok      github.com/go-spatial/geom/encoding/wkt 31.788s

usability

My implementation is a bit longer because it has to deal with a choice of removing or keeping empty geometries. It also returns a proper error message when lines and geometries with not enough valid points are sent for a polygon or linestring.

I just noticed there is also a bug, where empty points can be sent on a line string, this should not be allowed.

extra

I also took the liberty of changing all "Verticies" to "Vertices", I can remove this commit.

todo

ear7h commented 5 years ago

Agh also looks like I neglected to run package-wide tests

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 325


Changes Missing Coverage Covered Lines Changed/Added Lines %
encoding/mvt/feature.go 0 1 0.0%
planar/triangulate/gdey/quadedge/subdivision/subdivision.go 0 4 0.0%
encoding/wkt/wkt.go 9 15 60.0%
encoding/wkt/wkt_encode.go 312 429 72.73%
<!-- Total: 322 450 71.56% -->
Files with Coverage Reduction New Missed Lines %
encoding/wkt/wkt.go 3 57.14%
planar/triangulate/constraineddelaunay/triangulator.go 6 57.78%
<!-- Total: 9 -->
Totals Coverage Status
Change from base Build 287: 0.3%
Covered Lines: 11834
Relevant Lines: 20300

💛 - Coveralls
ear7h commented 5 years ago

@gdey yep and ci should pass. I removed the commit which implements build tags for tile test files. I will implement the solution we discussed in a different pr (#72)

update: it failed when running the vet command.