Problem with TopologyPreservingSimplifier.Simplify in multi threading context

[GoogleCodeExporter]

1/ What steps will reproduce the problem?
In multi threading context, TopologyPreservingSimplifier.Simplify doesn't 
compute the same results for the same geometry

2/ What is the expected output? What do you see instead?
In multi threading context, the expected output should be the same that in 
single context.
When I test, I obtain different results in multi threading context.

3/ What version of the product are you using? On what operating system?
I've tested NetTopologySuite v1.11.0.0 on :
- Windows XP pro SP3 (Pentium Dual core, 4Go RAM)
- Windows server 2003 32bits (6 physical cores amd opteron, 12 logical, 4Go RAM)
- Windows  7 Entreprise 64 (Core i5, 4 core)
=> More core leads to more inacurate results

4/ Please provide any additional information below.
I've written a VS2010 solution to reproduce the issue.

This solution executes the following steps:
1) Clean the working directory
2) Write a file named "NOT_SIMPLIFY_REFERENCE.txt" containing the coordinates 
of the line "L" which is the reference
3) Write a file named "SIMPLIFY_REFERENCE.txt" containing the coordinates of 
the result of only one simplification of the line "L"
4) Execute 100 simplifications of "L" in parallel tasks and generates 100 files 
named "SimplifyCoordinatesXXX.txt" (XXX is the number of the task) with each 
result.

You can see in the different generated files that:
1) The size of the file "NOT_SIMPLIFY_REFERENCE.txt" is 87 ko (ok)
2) The size of the file "SIMPLIFY_REFERENCE.txt" is 6 ko (the simplification is 
very good)
3) The sizes of the simplified files are between 10 ko and 24 ko : I think it 
is a bug (in my opinion it should always be 6 ko).

I hope it will help you to resolve this issue.

Regards,

Original issue reported on code.google.com by nicolas....@gmail.com on 29 Nov 2011 at 3:32

[GoogleCodeExporter]

I've implemented a workaround (lock in the private method void 
SimplifySection(int i, int j, int depth) of class 
NetTopologySuite.Simplify.TaggedLineStringSimplifier).

See the code below (the modified source is attached) :
public static string VERROU = "VERROU_TEST";

. . .

lock (VERROU)
{
      if (HasBadIntersection(_line, sectionIndex, candidateSeg))
      {
            isValidToFlatten = false;
      }
}

Original comment by nicolas....@gmail.com on 29 Nov 2011 at 4:00

Attachments:

• TaggedLineStringSimplifier.cs

[GoogleCodeExporter]

I think that the problem is the static readonly "li" object at the start of 
TaggedLineStringSimplifier: if you delete this item and use a brand new "li" 
inside "HasInteriorIntersection" all works well.

Original comment by diegogu...@gmail.com on 29 Nov 2011 at 4:15

• Changed state: Accepted

[GoogleCodeExporter]

fixed with r783. 
using your tests, all files now are 6k sized.
please take a look and see if now it's all ok also for you.

Original comment by diegogu...@gmail.com on 29 Nov 2011 at 4:26

• Changed state: Fixed

[GoogleCodeExporter]

Inside NTS code, we are A LOT of "static readonly" objects that should be 
removed... maybe we can open a separate issue for this.
P.S: thanks for reporting this isse. 

Original comment by diegogu...@gmail.com on 29 Nov 2011 at 4:28

[GoogleCodeExporter]

+1 for replacing static readonly objects.
I suspected it was the line intersector, you beat me to it :)

Original comment by felix.ob...@netcologne.de on 29 Nov 2011 at 6:21

[GoogleCodeExporter]

actually JTS too can have the same problem:
http://jts-topo-suite.svn.sourceforge.net/viewvc/jts-topo-suite/trunk/jts/java/s
rc/com/vividsolutions/jts/simplify/TaggedLineStringSimplifier.java?revision=482&
view=markup
maybe I can suggest the fix also to the JTS team...

Original comment by diegogu...@gmail.com on 30 Nov 2011 at 8:34

[GoogleCodeExporter]

fixed also FastSegmentSetIntersectionFinder and others.
all tests are green, hope no slower performances.

Original comment by diegogu...@gmail.com on 30 Nov 2011 at 9:29

[GoogleCodeExporter]

One question, why didn't you just remove the "static" keyword from the initial 
declaration of the li object? I don't know how "expensive" it is to create a 
LineIntersector class (doesn't look too heavy) but I think it should have been 
sufficient...

Original comment by felix.ob...@netcologne.de on 30 Nov 2011 at 9:52

[GoogleCodeExporter]

>why didn't you just remove the "static" keyword
Actually I don't thinked to this solution.
I suppose that should be enough, I try immediately and fix if works.

Original comment by diegogu...@gmail.com on 30 Nov 2011 at 9:56

[GoogleCodeExporter]

Done with r786

Original comment by diegogu...@gmail.com on 30 Nov 2011 at 10:07