geo: remove pointer usage for Geometry/Geography

[otan]

This should improve our heap profiles by making tree.DGeometry/Geography
only implant a geo.Geometry/Geography object.

Release justification: low risk, high reward changes to existing
functionality

Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Nice!

[nvanbenschoten]

Drive-by comment: this error is probably making the local vars escape back to the heap wherever it’s used. We might want to make the args values too.

[otan]

Might address that in a follow up PR. Gotta remove the interface and use spatial object instead.

[nvanbenschoten]

SGTM. I did just confirm that this is the case though, so you'll want to make that change before running sqlcmp. As is, this might actually be a pessimization.

[sumeerbhola]

Looks good.

• The sizeof a SpatialObject is currently 48 bytes. With some reduction in size of fields and rearrangement we can bring it down to 40 bytes.
  SRID: int32
  ShapeType: int32 but can be changed to int16
  Type: ditto
  EWKB: 24 bytes
  BoundingBox: 8 bytes
  If we reorder the fields as above, the first 3 fields get packed into an 8 byte word (the old ordering adds padding so no improvement).

• Can you run sqlcmp once with these changes just to make sure we don't have an unforeseen regression.

Reviewed 57 of 57 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

[otan]

The sizeof a SpatialObject is currently 48 bytes. With some reduction in size of fields and rearrangement we can bring it down to 40 bytes.

Does that change the encoding, or is protobuf okay with downgrading int sizes?

[nvanbenschoten]

EWKB: 24 bytes

If you're always parsing this using ewkb.Read, you can shave off 8 bytes by switching this to a string (i.e. an immutable byte slice). For this to work, you'd need to use a strings.Reader instead of the bytes.Reader we talked about before.

Doing so it likely not worth it if means you'd need to go from []byte to string and back often.

[otan]

good call @nvanbenschoten, after fixing the mismatching SRID pointer promotion:

name                                   old ms     new ms     delta
Test/postgis_geometry_tutorial/11.2b   3.88 ± 6%  2.88 ± 9%  -25.87%  (p=0.008 n=5+5)
Test/postgis_geometry_tutorial/11.6    3.80 ±11%  3.27 ±18%     ~     (p=0.222 n=5+5)
Test/postgis_geometry_tutorial/12.1.2  3.67 ±16%  2.15 ±16%  -41.34%  (p=0.008 n=5+5)
Test/postgis_geometry_tutorial/12.2.3  3.77 ±15%  2.98 ±21%  -21.11%  (p=0.032 n=5+5)
Test/postgis_geometry_tutorial/12.2.4  3.48 ±11%  3.44 ±67%     ~     (p=0.310 n=5+5)
Test/postgis_geometry_tutorial/13.0    4.05 ± 4%  3.17 ±18%  -21.67%  (p=0.008 n=5+5)
Test/postgis_geometry_tutorial/13.1a    428 ± 2%   412 ±13%     ~     (p=0.151 n=5+5)
Test/postgis_geometry_tutorial/13.1c   46.5 ± 8%  47.4 ±13%     ~     (p=0.841 n=5+5)
Test/postgis_geometry_tutorial/13.2     540 ± 4%   529 ± 8%     ~     (p=0.310 n=5+5)
Test/postgis_geometry_tutorial/14.1a   6.48 ± 5%  3.58 ± 7%  -44.77%  (p=0.008 n=5+5)
Test/postgis_geometry_tutorial/14.2b   19.8 ±35%  12.0 ±14%  -39.57%  (p=0.008 n=5+5)
Test/postgis_geometry_tutorial/14.2c   10.7 ±13%   6.8 ± 1%  -36.70%  (p=0.016 n=5+4)
Test/postgis_geometry_tutorial/14.3c   55.8 ±19%  47.7 ± 9%  -14.60%  (p=0.032 n=5+5)
Test/postgis_geometry_tutorial/15.0    4.11 ±22%  3.86 ±28%     ~     (p=0.421 n=5+5)

tacked on as a separate commit.

[sumeerbhola]

Does that change the encoding, or is protobuf okay with downgrading int sizes?

they are all varint encoded so should be ok, and we don't care about compatibility yet. But I overlooked two things: these are enums in the proto file; because of the varint encoding, there is no int16 type for protos. Maybe a proto extension can be used to tell gogoproto to do the right thing -- like gogoproto.casttype?

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @otan and @sumeerbhola)

[otan]

bors r=sumeerbhola

they are all varint encoded so should be ok, and we don't care about compatibility yet

I'm not sure -- I think we care once beta hits.
But arguably we can ask for an exemption in this case...

Maybe a proto extension can be used to tell gogoproto to do the right thing -- like gogoproto.casttype?

I think we can do that.

BoundingBox: 8 bytes

This escapes to the heap right now. If we can "flatten" it that may improve performance, but I'd need to be able "NULL" bounding box (maybe a bool or something).

[craig]

Build failed (retrying...):

• Essential CI (Cockroach)

[otan]

bors r=sumeerbhola

[otan]

bors r=sumeerbhola

[craig]

Build succeeded:

• Essential CI (Cockroach)

[sumeerbhola]

The sizeof a SpatialObject is currently 48 bytes. With some reduction in size of fields and rearrangement we can bring it down to 40 bytes.

btw, I tried this out but it made no difference to query latency
sumeerbhola@b688736