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

Added NTS support #1840

Closed
wants to merge 7 commits into from
Closed

Added NTS support #1840

wants to merge 7 commits into from

Conversation

YohDeadfall
Copy link
Contributor

@YohDeadfall YohDeadfall commented Mar 10, 2018

Known issues:

  • Doesn't support writing M because of these lines. It's NTS problem.

Closes #1816


Task WriteCore(IGeometry value, NpgsqlWriteBuffer buf, NpgsqlLengthCache lengthCache, NpgsqlParameter parameter, bool async)
{
_writer.Write(value, new WriteStream(buf));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably WriteStream should be cached like LengthStream.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you refactor it into NpgsqlWriteBuffer (see below) we could cache it there.

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not much to say - it's pretty straightforward and looks great. Take a look at my comments below, I think we can merge very soon.

#endif
}

sealed class WriteStream : Stream
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe refactor this into NpgsqlWriteBuffer, a bit like NpgsqlReadBuffer.GetStream()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea, but let's do that by an additional PR since it would be good to have tests for that stream.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you wish, no problem.

<PropertyGroup>
<VersionPrefix>1.0.0</VersionPrefix>
<Description>NetTopologySuite plugin for Npgsql, allowing mapping of PostGIS geometry types to NetTopologySuite types.</Description>
<Authors>Shay Rojansky</Authors>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget yourself :)


Task WriteCore(IGeometry value, NpgsqlWriteBuffer buf, NpgsqlLengthCache lengthCache, NpgsqlParameter parameter, bool async)
{
_writer.Write(value, new WriteStream(buf));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you refactor it into NpgsqlWriteBuffer (see below) we could cache it there.

@@ -13,9 +13,12 @@
<ProjectReference Include="../../src/Npgsql.Json.NET/Npgsql.Json.NET.csproj" />
<ProjectReference Include="../../src/Npgsql.NodaTime/Npgsql.NodaTime.csproj" />
<ProjectReference Include="../Npgsql.Tests/Npgsql.Tests.csproj" />
<ProjectReference Include="..\..\src\Npgsql.NetTopologySuite\Npgsql.NetTopologySuite.csproj" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reverse the slashes

/// <param name="mapper">The type mapper to set up (global or connection-specific)</param>
/// <param name="reader">The writer which consumes WKB input</param>
/// <param name="writer">The reader which produces WKB output</param>
public static INpgsqlTypeMapper UseNetTopologySuite(this INpgsqlTypeMapper mapper, IBinaryGeometryReader reader = null, IBinaryGeometryWriter writer = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it really make sense to expose the reader and writer to the user? In what scenario do you expect users to specify these?

Copy link
Contributor Author

@YohDeadfall YohDeadfall Mar 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense because someone may want to specify parameters for the reader. Same for the writer for symmetry.

public PostGisReader(IGeometryFactory factory);
public PostGisReader(ICoordinateSequenceFactory coordinateSequenceFactory, IPrecisionModel precisionModel);
public PostGisReader(ICoordinateSequenceFactory coordinateSequenceFactory, IPrecisionModel precisionModel, Ordinates handleOrdinates);

Otherwise, the ctor parameters should be exposed. Maybe it would be better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, makes total sense. And I think it's better exactly as you did it, accepting instances of IBinaryGeometryReader/IBinaryGeometryWriter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert or leave the current changes? See the Feedback commit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. UseNetTopologySuite() should accept IBinaryGeometry{Reader,Writer}, this allows maximum flexibility in case users need to do something odd
  2. If none are given, I'd guess the default should be PostGis{Reader,Writer} but I feel like we need to understand the differences between that and the generic WKB{Reader,Writer}. If all your tests pass on PostGis{Reader,Writer} and it seem to work well, let's go with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know why they need a few implementations, but from my point of view the difference only in bugs (: Will ask.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, mention me in those issues so I can follow the conversation too. The important thing is for users to be able to specify what they want, and for the default to work well.

}

protected override NpgsqlTypeHandler<IGeometry> Create(NpgsqlConnection conn)
=> new NetTopologySuiteHandler(_reader ?? new WKBReader(), _writer ?? new WKBWriter());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use the the PostGIS-specific ones? Any idea what PostGis2Reader does different/better than WKBReader? Maybe it supports the extra capabilities of PostGIS's EWKB or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I thought about the NetTopologySuite.IO.PostGis package, but it doesn't support .NET Standard. Therefore, I decided to use classes from NetTopologySuite.

Replaced by PostGisReader and PostGisWriter from NetTopologySuite.IO which targetes to .NET Standard.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um, PostGisReader has an problem. It uses Stream.Position to set the current position (source) which isn't supported by ColumnStream. I will help them fix the issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, in their develop branch NetTopologySuite.IO.PostGis targets netstandard1.0, although NetTopologySuite.IO.PostGis2 is an old-style csproj which doesn't. Would you like to take this up with them, understand the differences between PostGis1, PostGis2 and the generic WKBReader/Writer, and also push them to do netstandard for PostGis2 if relevant?

This isn't blocking and can also be explored and fixed post-merge.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the seeking (changing Stream.Position), remind them that PostGIS objects are theoretically streamed and cannot be assumed to be entirely in memory at any point...

</ItemGroup>

<ItemGroup>
<ProjectReference Include="..\Npgsql\Npgsql.csproj" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reverse the slashes

@roji
Copy link
Member

roji commented Mar 11, 2018

@YohDeadfall ping me when you want me to look again (and merge). Aside from the nitpicks above, for the more serious stuff (eg. WKBReader vs. PostGisReader by default) I'd rather we merge quickly and take care of it later.

Ordinates handleOrdinates = Ordinates.XYZM)
{
if (coordinateSequenceFactory == null)
coordinateSequenceFactory = GeometryServiceProvider.Instance.DefaultCoordinateSequenceFactory;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GeometryServiceProvider.Instance.DefaultCoordinateSequenceFactory is -unless previously configured otherwise- CoordinateArraySequenceFactory. This factory cannot hande measure ordinate values.

You have to choose another one if you want to allow XYZM;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Which factory should be used instead? DotSpatialAffineCoordinateSequenceFactory?

@YohDeadfall
Copy link
Contributor Author

@roji Is there a reason to use slashes instead of backslashes in paths?

@roji
Copy link
Member

roji commented Mar 20, 2018

@roji Is there a reason to use slashes instead of backslashes in paths?

I'm not sure, csproj seems to handle both, but being a Linux guy it hurts my eyes a little :) I guess the important thing is to at least be consistent about it...

@YohDeadfall
Copy link
Contributor Author

Tests are ignored.

I guess the important thing is to at least be consistent about it...

When you committed 08cc104 backslashes were introduced. I just think that it would be easier to keep backslashes if an IDE on Linux uses them.

@roji
Copy link
Member

roji commented Mar 20, 2018

Yeah, I know sometimes they slipped in... In any case it's no big deal.

So is this PR ready for review again? I'm really looking forward to merge support.

@YohDeadfall
Copy link
Contributor Author

Yep, it's waiting for you (:

@roji
Copy link
Member

roji commented Mar 28, 2018

Squashed, merged and made tiny amendments in f90ce07.

Thanks @YohDeadfall!

@roji roji closed this Mar 28, 2018
@YohDeadfall YohDeadfall deleted the NetTopologySuite branch March 28, 2018 07:25

protected override NpgsqlConnection OpenConnection(string connectionString = null)
{
NetTopologySuiteBootstrapper.Bootstrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YohDeadfall, this line seems mandatory, otherwise NTS doesn't work. Is there any reason to force users to do it rather than putting it in the plugin's UseNetTopologySuite()? If this is called twice is there any problem or is the 2nd time ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, the second call will replace the previous value of GeometryServiceProvider.Instance which could be set by an user. There is no check to do nothing if NTS is initialized already. See the source. But we can check if GeometryServiceProvider.Instance is set and call Bootstrap in UseNetTopologySuite.

public static INpgsqlTypeMapper UseNetTopologySuite(
    this INpgsqlTypeMapper mapper,
    ICoordinateSequenceFactory coordinateSequenceFactory = null,
    IPrecisionModel precisionModel = null,
    Ordinates handleOrdinates = Ordinates.None)
{
    IGeometryService geometryService;
    try
    {
        geometryService = GeometryServiceProvider.Instance;
    }
    catch (InvalidOperationException)
    {
        NetTopologySuiteBootstrapper.Bootstrap();
        geometryService = GeometryServiceProvider.Instance;
    }
    // other stuff
}

Yep, it's the check I mentioned. NTS throw an exception if it can't use a reflection based approach to initialize GeometryServiceProvider (source). So I think that users which use NTS know about the initialization and will do it by themselves.

@roji
Copy link
Member

roji commented Apr 9, 2018

First, what do you think about sending NTS a PR to make this better, at least to add a check if it's already initialized (or not overwriting an existing initialization, if that's better)?

The exception-based approach isn't ideal - for one it'll trigger debugger first-chance exceptions. I wouldn't assume anything about user knowledge, while working on the EF Core NTS plugin I lost a bit of time before I understood that I needed to bootstrap. Ideally just doing UseNetTopologySuite() should take care of everything...

@YohDeadfall
Copy link
Contributor Author

Yeah, it makes sense in that case, but at first it would be nice to invite @FObermaier and @airbreather to discuss.

I think it would be better to do other changes too. The Instance property should be redesigned a bit to remove locks while reading the field since it's an atomic operation for pointers. If it's null then lock while initializing the property using reflection.

If it would take a long time to merge then we can use the code above with a small change as an temporary solution. I mean that NpgsqlNetTopologySuiteExtensions can have a bool field to skip the try-catch block.

@airbreather
Copy link

The Instance property should be redesigned a bit to remove locks while reading the field since it's an atomic operation for pointers. If it's null then lock while initializing the property using reflection.

Yeah, this does look pretty bad. You're right that reads and writes of pointer-sized values are atomic, but that's not the only thing going on here. Unlike typical spots where lazy-init stuff comes in, it can be manually set, at any time, by any thread. The implicit memory barrier from lock ensures that you never have a case where one thread sets it one way and then another thread later sees a null and tries to initialize it.

I think simply marking the field volatile MIGHT be enough to guarantee that that can never happen. I'm not at all sure about this, and it seems safest to use lock (uncontended locks probably won't harm performance all that much, given what kinds of things usually happen to your CPU caches immediately after getting GeometryServiceProvider.Instance).

I'm not 100% sure about the right approach for bootstrapping Instance. A cool idea might be to include a module initializer in NTS that ensures that GeoAPI.NET's Instance is initialized to NTS's default if it's still uninitialized when NTS is first loaded... that would take care of the problems that @FObermaier alluded to over here without forcing anyone to explicitly call some undiscoverable method on their own.

I've filed NetTopologySuite/NetTopologySuite#227 since this feels like something that NTS (at least) shouldn't force everyone to have to deal with, especially given that its cousin JTS doesn't have this problem. At most, the only applications who should have to worry about this are:

  • those with awkward requirements like M ordinates or
  • those for which CoordinateArraySequence is simply too inefficient.

@airbreather
Copy link

airbreather commented Apr 9, 2018

Hmm, one thing I just thought of that worries me about the approach of having npgsql do its own bootstrap is a report that comes in sounding like this:

I was noticing blah blah blah issue with geometry from my database, but then when I went to try it using the original shapefiles / inline WKB byte array, I got an even worse error

and the reason for that inconsistency being that npgsql has its own bootstrap method.

Or even just that the user's code will sometimes work and sometimes not work, depending on whether they happen to try to load geometries from a shapefile or from the database first.

@roji
Copy link
Member

roji commented Apr 9, 2018

I think you want to look at Interlocked.Exchange() rather than volatile, which AFAIK is about preventing the optimization of your variable into a register and doesn't guarantee the propagation of a new value to code running concurrently on other cores.

I don't know much about NTS or about spatial, but as a dumb user I'd expect things to just work out of the box with a reasonable default without having to do any special initialization. Specific applications should be able to override this if they need to for specific needs, of course.

@YohDeadfall
Copy link
Contributor Author

YohDeadfall commented Apr 9, 2018

Interlocked.Exchange() isn't a good idea for long time initializations. Do it like Lazy<T> does with the ExecutionAndPublication mode: volatile read, check, return or initialize using a lock:

// volatile means that reading will be done using the Volatile.Read<T>(ref T) method
private static volatile IGeometryServices _instance;
private static readonly object _lock = new object();

public static IGeometryServices Instance
{
    get => _instance ?? InitializeInstance();
    set
    {
        if (value == null)
            throw new ArgumentNullException(nameof(value));
        lock (_lock)
           _instance = value;
    }
}

private static IGeometryServices InitializeInstance()
{
    lock (_lock)
    {
        var instance = _instance;
        if (instance != null) return instance;

        // other construction logic here
        return _instance = instance;
    }
}

The reason why the Instance getter is to short is that it will be automatically inlined because of the resulting IL size. The jitter does it if a method body is less or equal to 16 bytes.

@roji
Copy link
Member

roji commented Apr 9, 2018

@YohDeadfall your code looks good, I think I didn't have the proper context in mind.

@YohDeadfall
Copy link
Contributor Author

@airbreather What is a purpose of NtsGeometryServices.Instance if you have GeometryServiceProvider.Instance?

@airbreather
Copy link

What is a purpose of NtsGeometryServices.Instance if you have GeometryServiceProvider.Instance?

🤷‍♂️ both were there long before I started contributing to NTS.

@YohDeadfall
Copy link
Contributor Author

Who knows? I would like to eliminate it if possible and implement a new bootstrapper.

@FObermaier
Copy link

@YohDeadfall I plea guilty, but atm I have absolutely no time to recapture what lead me to the NtsGeometryServices.Instance. A new|Another bootstrapper mechanism is certainly appreciated if it better.

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.

4 participants