Refactor globals and GC mark them

[teckwan]

Summary

Refactor the globals, to make them global in C, and remove the link between factory and globals. This avoids passing references around, and treating ruby as a sort of storage for these globals.

Reproduction

The script below would segv every time (every rgeo / geos version that can compile nowadays)

pt=RGeo::Geos.factory.point(1,0)
RGeo::Geos::CAPIFactory::INTERNAL_CGLOBALS = nil
def polygon(pts, factory: RGeo::Geos.factory)
  pts = pts.each_slice(2) if pts.first.is_a?(Numeric) && pts.size.even?
  factory.polygon(
    ring(pts, factory: factory)
  )
end
polygon([0, 0,    0, 1,    1, 1,    1, 0,    0, 0]).contains?(pt)

Even though this is a pretty dumb script, pure ruby should not crash, moreover, using GC.compact a few times (or verify_compaction_references) would generate the same issue at some point.

[BuonOmo]

We extended the usage of this benchmark to "hash" and "cast" as well, the idea being that their usage does not call for such precise performance optimisation (otherwise, we should not be calling rb_funcall either).

Performance matters, however we're working with geometries that are much computation heavier than featching a value in a global symbol cache, hence we decided to pick our fights and prefer readability here.

The benchmark was made with:

require "benchmark/ips"
load "#{__dir__}/lib/rgeo.rb"

# Puts a Centered, bold and inverted text for better visibility.
def shout_out(string)
  puts "\n\e[7;1m#{string.center(75)}\e[27;0m\n\n"
end

shout_out("SMALL POLYGON")

def ring(pts, factory: RGeo::Geos.factory)
  factory.linear_ring(
    pts.map { factory.point(_1, _2) }
  )
end

def polygon(pts, factory: RGeo::Geos.factory)
  pts = pts.each_slice(2) if pts.first.is_a?(Numeric) && pts.size.even?
  factory.polygon(
    ring(pts, factory: factory)
  )
end

poly = polygon([0, 0,    0, 1,    1, 1,    1, 0,    0, 0])

Benchmark.ips do |x|
  x.config(time: 20)
  x.report("rb_intern") { poly == poly }
  x.report("cached") { poly == poly }
  x.hold!("temp")
  x.compare!
end

and commenting the polygon equality part to only have this class used in c.

[BuonOmo]

@teckwan I think we should update this comment

[BuonOmo]

shouldn't this be marked as well ? Note that however, in the current case, I did not find a way to generate a segv.

I tried some variations of :

RGeo::Geos::Utils.singleton_class.undef_method :psych_wkt_generator
RGeo::WKRep::WKTGenerator.undef_method :generate
RGeo::Geos::Utils = nil
f = RGeo::Geos.factory(has_z_coordinate: true)
pt = f.point(1, 0, 0);nil
GC.start
GC.verify_compaction_references(double_heap: true, toward: :empty)
f.write_for_psych(pt)

ObjectSpace.each_object(RGeo::WKRep::WKTGenerator).count # run this between every line to see evolution

At some point, I just found that RGeo::Geos.capi_supported? is false. It looks like there is an issue with our new organisation:

---

EDIT: my issue was local, a clean and compile fixed it. However:

f = RGeo::Geos.factory(has_z_coordinate: true)
pt = f.point(1, 0, 0);nil
GC.start
GC.verify_compaction_references(double_heap: true, toward: :empty)
f.write_for_psych(pt)

segfaults at some point, we'll have to mark it (i guess when setting it to Qnil)

[teckwan]

Effectively there was a need to mark this as well. However i think that since the value mutates, simply using rb_gc_register_mark_object was not sufficient (still segfaults using your reproduction). By using rb_gc_register_address it seems to be better.

[BuonOmo]

One last comment to add IMHO (reflecting the last answer you gave me) otherwise, LGTM!

@keithdoggett could you review this one as well ?

[BuonOmo]

Suggested change

	psych_wkt_generator = Qnil;
	rb_gc_register_address(&psych_wkt_generator);
	marshal_wkb_generator = Qnil;
	rb_gc_register_address(&marshal_wkb_generator);
	/* We favor rb_gc_register_address over rb_gc_register_mark_object because the value changes at runtime */
	psych_wkt_generator = Qnil;
	rb_gc_register_address(&psych_wkt_generator);
	marshal_wkb_generator = Qnil;
	rb_gc_register_address(&marshal_wkb_generator);

[keithdoggett]

yeah that makes sense to add a note. I didn't realize you could mark objects with the rb_gc_register_address and rb_gc_mark functions. Pretty cool

[keithdoggett]

This looks great thanks for the work on this! The notes mentioned in there make sense and this seems a lot better than passing around the globals struct.