This project has moved. For the latest updates, please go here.
2

Closed

Replace the slow Enum.IsDefined() call

description

I was profiling SharpMap 0.9 to see if I could squeeze out a bit more performance (the rendering speed isn't quite what I want--not yet, anyway :), and I noticed that this code in GeometryFromWKB.cs:

public static Geometry Parse(BinaryReader reader)
    {
        // Get the first Byte in the array.  This specifies if the WKB is in
        // XDR (big-endian) format of NDR (little-endian) format.
        Byte byteOrder = reader.ReadByte();

        if (!Enum.IsDefined(typeof(WkbByteOrder), byteOrder))
        {
            throw new ArgumentException("Byte order not recognized");
        }

        // Get the type of this geometry.
        UInt32 type = readUInt32(reader, (WkbByteOrder)byteOrder);

        if (!Enum.IsDefined(typeof(WkbGeometryType), type))
        {
            throw new ArgumentException("Geometry type not recognized");
        }
...I checked, and this code is also in SharpMap 2.0, although how and where it's used, I'm not so sure.

But one thing I'm sure of: this should be reworked to not use Enum.IsDefined, which is a very expensive operation (under the covers, it's doing all sorts of reflection, boxing, etc) and has a definite impact on my data processing perfomance. I changed this code to look something like:

public static Geometry Parse(BinaryReader reader)
    {
        // Get the first byte in the array.  This specifies if the WKB is in
        // XDR (big-endian) format of NDR (little-endian) format.
        WkbByteOrder byteOrder = (WkbByteOrder)reader.ReadByte();

        // Get the type of this geometry.
        WKBGeometryType type = (WKBGeometryType)ReadUInt32(reader, byteOrder);

        switch(type)
        {
            case WKBGeometryType.wkbPoint:
                return CreateWKBPoint(reader, byteOrder);
...and ReadUInt32() now throws an exception if the read byte is not defined by WkbByteOrder.Xdr, using an if/else if/else statement instead of Enum.IsDefined():
    private static uint ReadUInt32(BinaryReader reader, WkbByteOrder byteOrder)
    {
        if (byteOrder == WkbByteOrder.Xdr)
        {
            byte[] bytes = BitConverter.GetBytes(reader.ReadUInt32()); 
            Array.Reverse(bytes);
            return BitConverter.ToUInt32(bytes, 0);
        }
        else if( byteOrder == WkbByteOrder.Ndr)
        {
            return reader.ReadUInt32();
        }
        else
            throw new ArgumentException("Byte order not recognized");
    }
...thoughts?
Closed May 28, 2012 at 7:48 AM by petlof

comments

pauldendulk wrote Feb 13, 2008 at 11:20 AM

I tested your suggestions with the method below. First with the polygons countries.shp that is in the DemoWebSite App_Data. This was 16% faster. Then with points in cities.shp. This was 85% faster, nice.

Paul

static void Main(string[] args)
{
  ShapeFile shapeFile = new ShapeFile("data/cities.shp");
  shapeFile.Open();
  int iterations = 1000;

  int featureCount = shapeFile.GetFeatureCount();

  MemoryStream memoryStream = new MemoryStream();
  BinaryWriter binaryWriter = new BinaryWriter(memoryStream);

  for (uint i = 0; i < featureCount; i++)
  {
    byte[] bytes = GeometryToWKB.Write(shapeFile.GetFeature(i).Geometry);
    memoryStream.Write(bytes, 0, bytes.Length);
  }

  Geometry geometry;
  BinaryReader binaryReader = new BinaryReader(memoryStream);

  DateTime begin = DateTime.Now;
  for (uint j = 0; j < iterations; j++)
  {
    memoryStream.Position = 0;
    for (uint i = 0; i < featureCount; i++)
    {
      geometry = GeometryFromWKB.Parse(binaryReader);
    }
  }
  DateTime end = DateTime.Now;

  Console.WriteLine("Total time: " + end.Subtract(begin).TotalMilliseconds);
}

pauldendulk wrote Feb 13, 2008 at 4:16 PM

Resolved with changeset 30621.

pauldendulk wrote Feb 13, 2008 at 4:22 PM

I checked it in. I still do the Enum.IsDefined check in the default of the switch statement to distinguish between a Not Recognized and Not Supported exception.

Paul

pauldendulk wrote Feb 13, 2008 at 7:39 PM

** Closed by pauldendulk 2/13/2008 8:16 AM

codekaizen wrote Feb 13, 2008 at 7:39 PM

Reopened for v2.0

codekaizen wrote Feb 16, 2008 at 1:38 AM

In v2.0, the byte encoding routines were redone (they were terribly slow, too... see http://vectordotnet.blogspot.com/2008/01/swapping-byte-encoding-order-of-multi.html), so I converted it to the following:
        Byte byteOrder = reader.ReadByte();

        if (byteOrder != (Byte)WkbByteOrder.Xdr && byteOrder != (Byte)WkbByteOrder.Ndr)
        {
            throw new ArgumentException("Byte order not recognized.");
        }

        WkbByteOrder wkbByteOrder = (WkbByteOrder)byteOrder;

        // Get the type of this geometry.
        UInt32 type = readUInt32(reader, wkbByteOrder);
This way, the check occurs only once. I've only done this in v2.0 (in GeoAPI, since that is where the IO classes ended up for now).

kidjan wrote Feb 19, 2008 at 9:33 PM

Yeah, I'd noticed the Array.Reverse calls. The data I have doesn't ever hit that code, but given how slow BinaryReader.ReadDouble can be (my performance profiling now shows the bulk of activity in that code), it wouldn't surprise me if this would slow parsing by 50-100% or more.

Do you know if 2.0 has any plans to allow caching parsed data that's been read? In 0.9, this is the major slowdown; all of the data is sucked out of SQL Server and re-parsed every time the map is updated. For our application, the very first thing we do is display the entire map (to extents), so the entire database is parsed before the application even starts. Other applications might not benefit from that approach, but...seems like caching could drastically reduce the rendering times.

One other performance tweak could be to have rendering/parsing happen on separate threads.