WKT/WKB parsing performance optimizations

[oleksii-leonov]

@BuonOmo

Current ActiveRecord::ConnectionAdapters::PostGIS::OID::Spatial internal parsing uses:

          def wkt_parser(string)
            if binary_string?(string)
              RGeo::WKRep::WKBParser.new(spatial_factory, support_ewkb: true, default_srid: @factory_attrs[:srid])
            else
              RGeo::WKRep::WKTParser.new(spatial_factory, support_ewkt: true, default_srid: @factory_attrs[:srid])
            end
          end

RGeo::WKRep::WKBParser and RGeo::WKRep::WKTParser are pure Ruby implementations:

• https://github.com/rgeo/rgeo/blob/main/lib/rgeo/wkrep/wkt_parser.rb
• https://github.com/rgeo/rgeo/blob/main/lib/rgeo/wkrep/wkb_parser.rb

Another option is to use factory.parse_wkt() or factory.parse_wkb().

• When the factory is native GEOS CAPI, factory.parse_wkt() and factory.parse_wkb() internally proxy call the GEOS C function to parse the string into geometry: https://github.com/rgeo/rgeo/blob/80eed6d10b107a18b873aff0e98570b0bf0ed198/ext/geos_c_impl/factory.c#L243
• For FFI factories, it uses ::Geos::WktReader and ::Geos::WkbReader.
• For Ruby factories, it uses RGeo::WKRep::WKTParser and RGeo::WKRep::WKBParser.

GEOS factory with factory.parse_wkt() and factory.parse_wkb() is way faster than Ruby implementations in RGeo::WKRep::WKTParser and RGeo::WKRep::WKBParser.

For example:

large_circle = RGeo::Geographic.spherical_factory(buffer_resolution: 1000).point(0, 0).buffer(100)
large_circle_wkt = large_circle.as_text
large_circle_wkb = large_circle.as_binary
target_factory = RGeo::Geos.factory(srid: 4326)
n = 1_000

# WKT parsing
Benchmark.bm do |x|
  x.report("RGeo::WKRep::WKTParser") { n.times { RGeo::WKRep::WKTParser.new(target_factory, support_ewkt: true).parse(large_circle_wkt) } }
  x.report("GEOS factory.parse_wkt()") { n.times { target_factory.parse_wkt(large_circle_wkt) } }
end

#                               user     system      total        real
# RGeo::WKRep::WKTParser   20.830059   0.000542  20.830601 ( 20.865695)
# GEOS factory.parse_wkt()  0.842476   0.011024   0.853500 (  0.854785)

# WKB parsing
Benchmark.bm do |x|
  x.report("RGeo::WKRep::WKBParser") { n.times { RGeo::WKRep::WKBParser.new(target_factory, support_ewkb: true).parse(large_circle_wkb) } }
  x.report("GEOS factory.parse_wkb()") { n.times { target_factory.parse_wkb(large_circle_wkb) } }
end

#                               user     system      total        real
# RGeo::WKRep::WKBParser    8.811269   0.012583   8.823852 (  8.842664)
# GEOS factory.parse_wkb()  0.048067   0.016100   0.064167 (  0.064284)

So, GEOS WKT parsing is ~20x faster, and GEOS WKB parsing is ~150x faster.

On a scale of parsing 1 (even quite large) geometry, the difference is negligible. But in cases when we need to load tens or hundreds of thousands of geometries from the DB, the difference could be minutes or even hours.

In our project (we are using GEOS factories), we did a monkey-patch for ActiveRecord::ConnectionAdapters::PostGIS::OID::Spatial:

module ActiveRecord
  module ConnectionAdapters
    module PostGIS
      module OID
        class Spatial
          def parse_wkt(string)
            factory =
              if spatial_factory.is_a?(::RGeo::Feature::Factory::Instance)
                spatial_factory
              else
                spatial_factory.call(srid: factory_attrs.fetch(:srid))
              end

            if binary_string?(string)
              factory.parse_wkb(string)
            else
              factory.parse_wkt(string)
            end
          rescue RGeo::Error::ParseError, RGeo::Error::GeosError
            nil
          end
        end
      end
    end
  end
end

The original idea was taken from tneems@b587269.

I am not sure, maybe there are some nuances for FFI and Ruby factories (as we don't use them). I will try to test for those cases and open a PR.

[BuonOmo]

Yes please! I have just a doubt about the first part of your PR:

  factory =
    if spatial_factory.is_a?(::RGeo::Feature::Factory::Instance)
      spatial_factory
    else
      spatial_factory.call(srid: factory_attrs.fetch(:srid))
    end

Why do we need that?

Also, when I search for RGeo::WK in the repository, I see other usages, could we correct all of them ?

[totus]

@BuonOmo, to confirm the observation – for the "relatively empty" (up to 200 records) test database observed deviation is:

• Test suite execution with version 11.0 – 3.5s
• Test suite execution with version 11.1 – 24.5s

[oleksii-leonov]

Yes please! I have just a doubt about the first part of your PR:

factory =
if spatial_factory.is_a?(::RGeo::Feature::Factory::Instance)
spatial_factory
else
spatial_factory.call(srid: factory_attrs.fetch(:srid))
end
Why do we need that?

Also, when I search for RGeo::WK in the repository, I see other usages, could we correct all of them ?

@BuonOmo , I am also unsure if srid: factory_attrs.fetch(:srid) is really needed, will investigate deeper.

Also, when I search for RGeo::WK in the repository, I see other usages, could we correct all of them ?

Yes, it's the right thing to do.

The factory should own its WKT/WKB parsing implementation. We should just call factory.parse_wkt/factory.parse_wkb instead of supplying behaviour with RGeo::WKRep::WKTParser/RGeo::WKRep::WKBParser manually.

[oleksii-leonov]

@BuonOmo, to confirm the observation – for the "relatively empty" (up to 200 records) test database observed deviation is:

• Test suite execution with version 11.0 – 3.5s
• Test suite execution with version 11.1 – 24.5s

@totus

Hm, it looks unrelated to the topic (since WKT/WKB parsing in 11.0 and 11.1 wasn't changed).

But there were other changes in 11.1, which means there is another reason for the slowdown. It would be great if you could make reproducible test cases. For example, put 10_000 geometries in the DB, load using 11.0 and 11.1. If the difference persists, we could use a vernier (or other profilers) to investigate.

[BuonOmo]

But there were other changes in 11.1, which means there is another reason for the slowdown. It would be great if you could make reproducible test cases. For example, put 10_000 geometries in the DB, load using 11.0 and 11.1. If the difference persists, we could use a vernier (or other profilers) to investigate.

The main difference is that we instanciate a lot more factories (since we don't keep them cached for ractor compatibility purposes, and RGeo is far from having immutable objects). We could still cache those factories though, for instance using the Deduplicable rails module. Please, feel free to have a go at this @totus. And as said @oleksii-leonov those are two different tasks :)

The factory should own its WKT/WKB parsing implementation. We should just call factory.parse_wkt/factory.parse_wkb instead of supplying behaviour with RGeo::WKRep::WKTParser/RGeo::WKRep::WKBParser manually.

Well, we could also consider changing RGeo's API, or make it understand which parser should be used. But the scope would be way wider, so we'd have to see if many ppl would benefit this, otherwise it is definitely simpler to stay in this library's scope.

Thank you both for investigating, It seems like we are going to have quite a great performance update soon 💪 🚀