lib2geom-devel — development discussion

Re: [Lib2geom-devel] Path translations

[Krzysztof K. <twe...@gm...>]

2013/11/13 Johan Engelen <jbc...@sw...>:
> This is not possible. Because curves are not closed under Affine transform,
> there is no operator*=(Affine). If you want to transform a Curve, you have
> to call
>     Curve* Curve::transformed(Affine const&)
>
> I'll clean up the work and commit my changes, so you can see.

Oops, I forgot about HLineSegment / VLineSegment.

I wonder whether keeping these as separate classes is worth breaking
the assumption that curves are closed under arbitrary affines, not
only translation and scaling.

Regards, Krzysztof

Re: [Lib2geom-devel] Path translations

[Johan E. <jbc...@sw...>]

On 13-11-2013 16:50, Krzysztof Kosiński wrote:
> 2013/11/12 Johan Engelen <jbc...@sw...>:
>> As far as I can see now, all (current) Curve types *are* closed under
>> translations. I think we should enforce this for Curve. I.e., each Curve
>> must implement
>>          virtual Curve &operator+=(Point const &);
> I think the method should be:
> virtual Curve &operator*=(Translate const &t) {
>      *this *= Affine(t);
>      return *this;
> }
>
> Rationale:
> 1. It's not immediately obvious what adding a point to a curve means.
> For instance, for BezierCurve it might plausibly mean adding a control
> point.

Ok, that makes sense.

> 2. For curves that don't implement the specialization, we just
> multiply by a generic affine.

This is not possible. Because curves are not closed under Affine 
transform, there is no operator*=(Affine). If you want to transform a 
Curve, you have to call
     Curve* Curve::transformed(Affine const&)

I'll clean up the work and commit my changes, so you can see.

Cheers,
   Johan

Re: [Lib2geom-devel] Path translations

[Krzysztof K. <twe...@gm...>]

2013/11/12 Johan Engelen <jbc...@sw...>:
> As far as I can see now, all (current) Curve types *are* closed under
> translations. I think we should enforce this for Curve. I.e., each Curve
> must implement
>         virtual Curve &operator+=(Point const &);

I think the method should be:
virtual Curve &operator*=(Translate const &t) {
    *this *= Affine(t);
    return *this;
}

Rationale:
1. It's not immediately obvious what adding a point to a curve means.
For instance, for BezierCurve it might plausibly mean adding a control
point.
2. For curves that don't implement the specialization, we just
multiply by a generic affine.

Regards, Krzysztof

Re: [Lib2geom-devel] Bug in elliptical-arg?

[Krzysztof K. <twe...@gm...>]

2013/11/7 Johan Engelen <jbc...@sw...>:
>  From symmetry of the code, shouldn't "dsq" be "dsq1" in that line? The
> code compiles because earlier, "dsq" was defined. If this is not a bug,
> please add a comment in the code, saying that it is indeed correct.
> If it *is* a bug, please correct it, and also reduce the scope of the
> "dsq" variable defined earlier.

That also looks like a bug to me.

Probably the best way to verify this is to write some tests.

Regards, Krzysztof

Re: [Lib2geom-devel] Path translations

[<jbc...@sw...>]

Hi Nathan,
  To be frank, I'd appreciate it if you'd read my first mail again, and look up the relevant bits of code, because your reply does not make much sense ;-) :P

Short summary:
1.  Is it OK to add the (first non-const) member function to Curve:  Curve::operator+=(Point)
     - This *forces* all Curve-descendants to be closed under Translation. This is the case for all current descendants, I'd like a second opinion on a possible design error that hurts us later on.
2.  Path currently stores its Curves in shared_ptr<vector<shared_ptr<const Curve>>>
     For more efficient Translates, the "const" is problematic and currently I have to used a const_cast. If we remove the const, we loose an important compile-time check, but the const_cast is pretty ugly I find...

Thanks,
  Johan



---- Nathan Hurst <nj...@nj...> wrote: 
> I thought we had specialised Matrix types translate, scale etc which
> you could just specialise the templates on.  (indeed I think if you
> use templates here it should 'just work')
> 
> njh
> 
> On Tue, Nov 12, 2013 at 01:18:16AM +0100, Johan Engelen wrote:
> > Hi all,
> >    Currently, when translating a path (adding a point), 2geom treats it 
> > as any other affine transform. This is quite a performance hit.
> > 
> > (refresher:
> > Path is a collection of Curves. A Curve is a virtual base for many curve 
> > types. Because many curve types are not closed under affine transforms 
> > (the result after an affine transform can not always be represented as 
> > its original Curve type, e.g. HLineSegment after rotation), when 
> > transforming a Curve, a new Curve is created that holds the transform. 
> > When transforming a path, each Curve is transformed, and thus a bunch of 
> > new Curves are created.
> > )
> > 
> > As far as I can see now, all (current) Curve types *are* closed under 
> > translations. I think we should enforce this for Curve. I.e., each Curve 
> > must implement
> >         virtual Curve &operator+=(Point const &);
> > I tested the performance increase for special-casing Translate 
> > transforms, by adding
> >        Path &operator*=(Translate const &m) {
> >          *this += m.vector();
> >          return *this;
> >        };
> >        Path &operator+=(Point const &m);
> > next to the current
> >        Path &operator*=(Affine const &m);
> > Of course, more changes were needed, and so far I have only implemented 
> > addition for BezierCurveN, just to see the performance increase.
> > 
> > After the change, the path-operations test runs more than twice as fast.
> > 
> > Do you have any concerns about enforcing an operator+= implementation 
> > for Curves ?
> > Interestingly, *all* member functions of Curve are const.
> > operator+= would be the first state-changing member function. :)
> > 
> > Cheers,
> >    Johan
> > 
> > 
> > ------------------------------------------------------------------------------
> > November Webinars for C, C++, Fortran Developers
> > Accelerate application performance with scalable programming models. Explore
> > techniques for threading, error checking, porting, and tuning. Get the most 
> > from the latest Intel processors and coprocessors. See abstracts and register
> > http://pubads.g.doubleclick.net/gampad/clk?id=60136231&iu=/4140/ostg.clktrk
> > _______________________________________________
> > Lib2geom-devel mailing list
> > Lib...@li...
> > https://lists.sourceforge.net/lists/listinfo/lib2geom-devel

Re: [Lib2geom-devel] Path translations

[Nathan H. <nj...@nj...>]

I thought we had specialised Matrix types translate, scale etc which
you could just specialise the templates on.  (indeed I think if you
use templates here it should 'just work')

njh

On Tue, Nov 12, 2013 at 01:18:16AM +0100, Johan Engelen wrote:
> Hi all,
>    Currently, when translating a path (adding a point), 2geom treats it 
> as any other affine transform. This is quite a performance hit.
> 
> (refresher:
> Path is a collection of Curves. A Curve is a virtual base for many curve 
> types. Because many curve types are not closed under affine transforms 
> (the result after an affine transform can not always be represented as 
> its original Curve type, e.g. HLineSegment after rotation), when 
> transforming a Curve, a new Curve is created that holds the transform. 
> When transforming a path, each Curve is transformed, and thus a bunch of 
> new Curves are created.
> )
> 
> As far as I can see now, all (current) Curve types *are* closed under 
> translations. I think we should enforce this for Curve. I.e., each Curve 
> must implement
>         virtual Curve &operator+=(Point const &);
> I tested the performance increase for special-casing Translate 
> transforms, by adding
>        Path &operator*=(Translate const &m) {
>          *this += m.vector();
>          return *this;
>        };
>        Path &operator+=(Point const &m);
> next to the current
>        Path &operator*=(Affine const &m);
> Of course, more changes were needed, and so far I have only implemented 
> addition for BezierCurveN, just to see the performance increase.
> 
> After the change, the path-operations test runs more than twice as fast.
> 
> Do you have any concerns about enforcing an operator+= implementation 
> for Curves ?
> Interestingly, *all* member functions of Curve are const.
> operator+= would be the first state-changing member function. :)
> 
> Cheers,
>    Johan
> 
> 
> ------------------------------------------------------------------------------
> November Webinars for C, C++, Fortran Developers
> Accelerate application performance with scalable programming models. Explore
> techniques for threading, error checking, porting, and tuning. Get the most 
> from the latest Intel processors and coprocessors. See abstracts and register
> http://pubads.g.doubleclick.net/gampad/clk?id=60136231&iu=/4140/ostg.clktrk
> _______________________________________________
> Lib2geom-devel mailing list
> Lib...@li...
> https://lists.sourceforge.net/lists/listinfo/lib2geom-devel

[Lib2geom-devel] Path translations

[Johan E. <jbc...@sw...>]

Hi all,
   Currently, when translating a path (adding a point), 2geom treats it 
as any other affine transform. This is quite a performance hit.

(refresher:
Path is a collection of Curves. A Curve is a virtual base for many curve 
types. Because many curve types are not closed under affine transforms 
(the result after an affine transform can not always be represented as 
its original Curve type, e.g. HLineSegment after rotation), when 
transforming a Curve, a new Curve is created that holds the transform. 
When transforming a path, each Curve is transformed, and thus a bunch of 
new Curves are created.
)

As far as I can see now, all (current) Curve types *are* closed under 
translations. I think we should enforce this for Curve. I.e., each Curve 
must implement
        virtual Curve &operator+=(Point const &);
I tested the performance increase for special-casing Translate 
transforms, by adding
       Path &operator*=(Translate const &m) {
         *this += m.vector();
         return *this;
       };
       Path &operator+=(Point const &m);
next to the current
       Path &operator*=(Affine const &m);
Of course, more changes were needed, and so far I have only implemented 
addition for BezierCurveN, just to see the performance increase.

After the change, the path-operations test runs more than twice as fast.

Do you have any concerns about enforcing an operator+= implementation 
for Curves ?
Interestingly, *all* member functions of Curve are const.
operator+= would be the first state-changing member function. :)

Cheers,
   Johan

Re: [Lib2geom-devel] C++11 initial performance test

[Nathan H. <nj...@nj...>]

On Fri, Nov 08, 2013 at 01:06:53AM -0800, Josh Andler wrote:
> On Nov 8, 2013 12:05 AM, "Jasper van de Gronde" <th....@hc...>
> wrote:
> > SVG d string writing and reading is indeed relatively slow, and frankly
> > we shouldn't be doing it all the time (also leads to precision issues).
> > But good work getting some testing done and spending time on making
> > lib2geom maintainable!
> 
> Writing helps with emergency saves though, right? Maybe reading it is more
> problematic? Shouldn't we write and not bother rereading unless
> necessary?

We should also do the writing in a background thread with low priority
- it should be able to 'catch up' fast enough for most people.

njh

Re: [Lib2geom-devel] C++11 initial performance test

[Josh A. <sc...@gm...>]

On Nov 8, 2013 12:05 AM, "Jasper van de Gronde" <th....@hc...>
wrote:
> SVG d string writing and reading is indeed relatively slow, and frankly
> we shouldn't be doing it all the time (also leads to precision issues).
> But good work getting some testing done and spending time on making
> lib2geom maintainable!

Writing helps with emergency saves though, right? Maybe reading it is more
problematic? Shouldn't we write and not bother rereading unless necessary?

Cheers,
Josh

Re: [Lib2geom-devel] C++11 initial performance test

[Jasper v. de G. <th....@hc...>]

On 2013-11-07 23:22, Johan Engelen wrote:
> ...
> Today, I simply added a piece of relatively complicated 2Geom code from
> Inkscape. It is the Bend Path LPE code, slightly simplified by removing
> some unimportant parameters. It turns out, that code is pretty fast (4
> ms), even though it may seem slow in Inkscape. So the "slowness" in
> Inkscape is elsewhere, perhaps the SVG d string writing and reading.
SVG d string writing and reading is indeed relatively slow, and frankly 
we shouldn't be doing it all the time (also leads to precision issues). 
But good work getting some testing done and spending time on making 
lib2geom maintainable!

Re: [Lib2geom-devel] performance tests

[mathog <ma...@ca...>]

On 07-Nov-2013 14:09, Johan Engelen wrote:

> I highly recommend using Ninja for building instead of Make.

Not broken, don't fix.

> I'd like some input in how to improve the performance test framework. I
> believe we want separate .cpp files for each performance test. Each
> performance test should take a couple of 100 ms. And then each test
> should be done a couple of times (three times currently) to get some
> averaging going.

Very hard to generalize the best way to do performance testing on a 
library.

Assume we are talking about purely algorithmic testing, so that 
interference from screen synchronization events is eliminated.

The biggest issue you are likely to run into is that small very fast 
routines are extremely difficult to measure accurately.  As a rule of 
thumb, if the execution time within a routine is of the same order of 
magnitude as a typical function call, then the measurement will be poor.

The other issue you will hit is that the order of function tests can 
dramatically change the measurement.  This happens because depending on 
how the program executes data and code is progressively forced out of 
the fastest memory into slower and slower memory, in a really bad case, 
all the way into virtual memory swapping to disk.  These memory effects 
cover orders of magnitude in speed, which can totally swamp any intended 
measurements of algorithm speed.

All else being equal then, you want the test code to beat on a single 
routine very many times (at least thousands for fast routines, fewer for 
slow routines), before moving on to test the next routine.  Within one 
routine test loop it is OK to vary the parameters, so long as the method 
for doing so is itself very fast (relative to the execution speed within 
the routine).

3 is too small a number for any test.  Consider what happens when you 
run
an arbitrary fast test program 3 times right after the system boots up.  
The first run time will consist almost entirely of the OS pulling in 
libraries and other codes before it even starts to run the program.  The 
2nd and 3rd runs, assuming everything stays in memory, will not have 
this delay.  Wait a couple of hours and run it again.  Is it still in 
memory?  This is crude, but one way around that issue is to run the test 
program once without timing it, then start the timing loop.

Regards,

David Mathog
ma...@ca...
Manager, Sequence Analysis Facility, Biology Division, Caltech

[Lib2geom-devel] C++11 initial performance test

[Johan E. <jbc...@sw...>]

Hi all,
   Just to poke some fun, please don't take things too seriously right now.

Yesterday, I tried to write code that should run much faster in C++11. I 
failed. It performed the same with or without "-std=gnu++0x". I think 
part of the reason is that our Path class is already optimized for 
copying, and so ... whatever. :-) The code did not become slower, so 
that's good too.

Today, I simply added a piece of relatively complicated 2Geom code from 
Inkscape. It is the Bend Path LPE code, slightly simplified by removing 
some unimportant parameters. It turns out, that code is pretty fast (4 
ms), even though it may seem slow in Inkscape. So the "slowness" in 
Inkscape is elsewhere, perhaps the SVG d string writing and reading. 
Anyhow, what is the change in performance between C++11 on or off? Same 
code, just recompile 2Geom with C++11 on or off.
The C++11 version is 10% faster. It is an interesting result. The same 
(user) code is suddenly 10% faster. The code is not exactly the same: 
the STL and possibly other GCC libraries do have C++11 code in it that 
is turned on and off (e.g. move semantics for containers).
Perhaps I can use this as an argument to convince people to switch to 
C++11 (at least partly). ;-)

Hope I can inspire you guys to add more performance tests. I'd like to 
see tests for single functions (bounding box, arc-length-reparam, etc.) 
and also complicated pieces like LPE Bend Path.

I have not yet tried with clang. Only gcc 4.6.1 on Windows.

Thanks!
   Johan

[Lib2geom-devel] performance tests

[Johan E. <jbc...@sw...>]

Hi all,
   I added a "perf" build target and two simple performance tests to trunk.
I highly recommend using Ninja for building instead of Make. Because we 
have so many subtargets, Ninja is much much faster when recompiling a 
piece of code. To generate Ninja build files: "cmake -G Ninja .....".
It is pretty fun to write some test code, tweak it, and meanwhile run 
"ninja perf" to see if your changes have any effect (as you all know, 
the result is generally the opposite of what you'd expect).

I'd like some input in how to improve the performance test framework. I 
believe we want separate .cpp files for each performance test. Each 
performance test should take a couple of 100 ms. And then each test 
should be done a couple of times (three times currently) to get some 
averaging going.
To reduce the amount of duplicated code, how about I make a 
PerformanceTest base class, and let each performance test derive from 
that? The PerformanceTest class would have boilerplate and a
       virtual clock_t run_test()
  method that returns the clockticks for that test. Then all outputting 
is done in the base class, and the main of each test would look like:
     int main() {
         DerivedTest test;
         test.run();
     }
We could even add some standard paths and other "inputs" inside the 
baseclass, ready to be used by performance test code.

I need your input on this. Yes, I am duplicating functionality of 
existing test frameworks. But I'd like to keep it simple and 
understandable. It doesn't have to be a very powerful system (imho).

Cheers,
   Johan

[Lib2geom-devel] Bug in elliptical-arg?

[Johan E. <jbc...@sw...>]

Hi all,
   Is there a bug perhaps in this piece of code:
elliptical-arc.cpp, lines 608-632

     double dsq1 = distanceSq(p, pointAt(from));
     double dsq2 = distanceSq(p, pointAt(to));
     if ( second_sol )
     {
         if ( mindistsq2 > dsq1 )
         {
             result.clear();
             result.push_back(from);
             mindistsq2 = dsq1;
         }
         else if ( are_near(mindistsq2, dsq) )    <--------- BUG ?
         {
             result.push_back(from);
         }
         if ( mindistsq2 > dsq2 )
         {
             result.clear();
             result.push_back(to);
         }
         else if ( are_near(mindistsq2, dsq2) )
         {
             result.push_back(to);
         }

     }

 From symmetry of the code, shouldn't "dsq" be "dsq1" in that line? The 
code compiles because earlier, "dsq" was defined. If this is not a bug, 
please add a comment in the code, saying that it is indeed correct.
If it *is* a bug, please correct it, and also reduce the scope of the 
"dsq" variable defined earlier.

Thanks a bunch!
   Johan

Re: [Lib2geom-devel] C-arrays

[Josh A. <sc...@gm...>]

On Wed, Nov 6, 2013 at 5:16 PM, Krzysztof Kosiński <twe...@gm...> wrote:
> In Debian, CGAL depends on a ton of stuff, including Qt. It also looks
> somewhat harder to use than 2Geom. However I don't have a strong
> opinion on using it.

I don't have a strong opinion myself, just a desire for us to get
access to better boolops and to get closer to being able to ditch
livarot.

The dependencies might have changed more recently... in addition to
"libcgal10", there is also a "libcgal-qt4-10". On Ubuntu 13.10,
according to Synaptic here are the deps for "libcgal10":

libboost-thread1.53.0
libc6
libgcc1
libgl1-mesa-glx | libgl1
libgmp10
libstdc++6
zlib1g

It's not even recommending or suggesting Qt on that main package. The
changelog doesn't give me any reason to believe it's an Ubuntu
specific change either.


> FWIW, the documentation of the boolops code in CGAL is here. CGAL
> subdivides the input curves into x-monotone fragments, so we would
> need to "un-subdivide" the result after it is computed.
> http://doc.cgal.org/latest/Boolean_set_operations_2/index.html

Great, more things to learn that I will likely never have the full
knowledge needed to be able to apply them anywhere. :-)

Cheers,
Josh

Re: [Lib2geom-devel] C-arrays

[Nathan H. <nj...@nj...>]

On Thu, Nov 07, 2013 at 01:48:30AM +0100, Krzysztof Kosiński wrote:
> 2013/11/6 Nathan Hurst <nj...@nj...>:
> > I would rather we focused on careful profiling and optimisation.
> >
> > njh
> 
> I had the impression that the performance of 2Geom is not a bottleneck
> in any of its uses within Inkscape.
> 
> In the few cases where geometry calculations are slow, it's either a
> case of using livarot, or a case of gratuitously recomputing values
> which could be cached.

Interesting!  I'd love to have conclusive proof of that, but you
asserting it is halfway there :)

njh

Re: [Lib2geom-devel] C-arrays

[Krzysztof K. <twe...@gm...>]

2013/11/7 Josh Andler <sc...@gm...>:
> It looks to me like cgal is decently active (8 GSoC projects this
> year) ... any reason this shouldn't be looked into further?

In Debian, CGAL depends on a ton of stuff, including Qt. It also looks
somewhat harder to use than 2Geom. However I don't have a strong
opinion on using it.

FWIW, the documentation of the boolops code in CGAL is here. CGAL
subdivides the input curves into x-monotone fragments, so we would
need to "un-subdivide" the result after it is computed.
http://doc.cgal.org/latest/Boolean_set_operations_2/index.html

Regards, Krzysztof

Re: [Lib2geom-devel] C-arrays

[Josh A. <sc...@gm...>]

On Wed, Nov 6, 2013 at 8:39 AM, Nathan Hurst <nj...@nj...> wrote:
> as far as bad is concerned, I would rather we worked on improving the
> api and numerical guarantees for other parts of the code.  What are
> the main reasons that we haven't completely moved to 2geom in
> inkscape?  Does someone want to take on boolops again?  Even porting
> the livarot boolops to 2geom notation would be beneficial.
>
> Another direction we could go is switching to cgal for the topology
> operators.  Now that cgal has a compatible licence we need to
> reconsider it.  The problems we had in the past (IIRC) where the
> licence, performance and accuracy.  It is likely that these are all no
> longer issues.

Other than the renderer (which has since been done) and boolops, I
recall that the tweak tool was one thing bulia had mentioned a while
back should be changed to use 2geom. If anything, I don't know if
there are any other good reasons to give than nobody has really had an
itch to convert what is left. :-/

It looks to me like cgal is decently active (8 GSoC projects this
year) ... any reason this shouldn't be looked into further?

Cheers,
Josh

Re: [Lib2geom-devel] C-arrays

[Krzysztof K. <twe...@gm...>]

2013/11/6 Nathan Hurst <nj...@nj...>:
> I would rather we focused on careful profiling and optimisation.
>
> njh

I had the impression that the performance of 2Geom is not a bottleneck
in any of its uses within Inkscape.

In the few cases where geometry calculations are slow, it's either a
case of using livarot, or a case of gratuitously recomputing values
which could be cached.

Regards, Krzysztof

Re: [Lib2geom-devel] C-arrays

[Krzysztof K. <twe...@gm...>]

My take:

1. bezier-utils.cpp does not actually "work". An important part of the
algorithm (sampling the curves to replace) is missing and must be
implemented externally. Rewriting it using 2Geom types would make it
easier to improve it.
http://inkscape.13.x6.nabble.com/Preserving-shapes-better-when-deleting-nodes-td4966563.html

2. I think that ease of use is very important. What I would like to
see is: I pass iterators denoting a range of some path, I get
BezierCurve that approximates that part of the path. Currently this is
nowhere near as easy.

Regards, Krzysztof

Re: [Lib2geom-devel] C-arrays

[Johan E. <jbc...@sw...>]

On 6-11-2013 20:42, mathog wrote:
> On 06-Nov-2013 10:44, Johan Engelen wrote:
>
>> If anything, it will improve the readability of the code (in my
>> opinion), which may inspire someone to further improve the code, or to
>> use it somewhere else in the lib. Why do you think it will not improve
>> the code? (certainly for stability/maintainability there is an obvious
>> gain, perhaps you missed it in my earlier mails)
> No, I didn't miss it, I just don't believe it.  I have been hearing
> claims like this for the last 35 or so years, for one language (or
> version) after another, and the outcome of rewriting code for these
> sorts of stylistic reasons has rarely resulted in any of the claimed
> benefits.  Working code is best left alone.  If 2geom was an
> incomprehensible mess of spaghetti code you might have a point, but I
> just had a look at it, and its organization seems pretty typical for C++
> code.
>
> Now if you have a functional improvement to make to 2geom, by which I
> mean changing some method to a different algorithm that runs faster or
> uses less memory, or adding a needed method, then by all means go ahead.
>    But don't waste your time rewriting working code just to make it better
> conform to some stylistic ideal.

I will stop trying to argue with someone who does not develop 2geom, 
does not stay on topic, and does not reply to arguments put forth.

-Johan

Re: [Lib2geom-devel] C-arrays

[mathog <ma...@ca...>]

On 06-Nov-2013 10:44, Johan Engelen wrote:

> If anything, it will improve the readability of the code (in my
> opinion), which may inspire someone to further improve the code, or to
> use it somewhere else in the lib. Why do you think it will not improve
> the code? (certainly for stability/maintainability there is an obvious
> gain, perhaps you missed it in my earlier mails)

No, I didn't miss it, I just don't believe it.  I have been hearing 
claims like this for the last 35 or so years, for one language (or 
version) after another, and the outcome of rewriting code for these 
sorts of stylistic reasons has rarely resulted in any of the claimed 
benefits.  Working code is best left alone.  If 2geom was an 
incomprehensible mess of spaghetti code you might have a point, but I 
just had a look at it, and its organization seems pretty typical for C++ 
code.

Now if you have a functional improvement to make to 2geom, by which I 
mean changing some method to a different algorithm that runs faster or 
uses less memory, or adding a needed method, then by all means go ahead. 
  But don't waste your time rewriting working code just to make it better 
conform to some stylistic ideal.

Regards,

David Mathog
ma...@ca...
Manager, Sequence Analysis Facility, Biology Division, Caltech

Re: [Lib2geom-devel] C-arrays

[Johan E. <jbc...@sw...>]

On 6-11-2013 17:39, Nathan Hurst wrote:
> I 100% support your position, but having been down this path in the
> past I strongly advise you to take care:
>
> On Wed, Nov 06, 2013 at 05:11:33PM +0100, jbc...@sw... wrote:
>> Yes, we can work on performance optimization, but I think that's
>> pointless for code that is in a "bad" state. Note that vectors do
>> not make code slow at all
> That is not true in my experience.  Sure, there is negligible overhead
> for the access of the vector vs array, but there is definitely
> measurable overhead in the more complicated allocator.

To clear the confusion: I was talking mainly about the dynamically 
allocated C-arrays that are new'ed in that code.


> as far as bad is concerned, I would rather we worked on improving the
> api and numerical guarantees for other parts of the code.  What are
> the main reasons that we haven't completely moved to 2geom in
> inkscape?  Does someone want to take on boolops again?  Even porting
> the livarot boolops to 2geom notation would be beneficial.
>
> Another direction we could go is switching to cgal for the topology
> operators.  Now that cgal has a compatible licence we need to
> reconsider it.  The problems we had in the past (IIRC) where the
> licence, performance and accuracy.  It is likely that these are all no
> longer issues.

This will also improve the API, making it more in-sync with the rest of 
2geom.
I think boolops is a separate discussion / goal. It requires a different 
skill-set too :-)
I agree that any work on boolops would be great.

>> , and when we better integrate this
>> particular piece of code using the 'standard' 2geom types, it might
>> have the side effect of making things a little bit faster because of
>> less conversion between data structures. So my plan was: C++ify
>> code, fix the bugs that show up, see what can be improved
>> performance-wise and interface-wise.
> That seems backwards - first profile and find the problems, then fix
> the code.

I am somewhat surprised at the performance concern. There is no reason 
at all to believe the changes I propose would make code perform 
noticeably worse or better.
But yes, I already thought about first writing performance tests, just 
to prove that it won't make a difference.

>> By the way, a big reason for me to switch to C++11 is to see if it
>> would improve performance (e.g. move semantics) while maintaining
>> our high-level of abstraction. I guess we can start writing
>> performance tests/toys, to see if the changes really bring
>> anything. The new C++11 <chrono> library seems to provide portable
>> high-precision clock... I have not tested it yet though.
> We can use C++11 in 2geom for tests and leave the library itself in C++99

Yep, will do that.

Cheers,
   Johan

Re: [Lib2geom-devel] C-arrays

[Johan E. <jbc...@sw...>]

On 6-11-2013 18:23, mathog wrote:
> On 06-Nov-2013 08:39, Nathan Hurst wrote:
>
>> But that code has been used for a decade now with no evidence of
>> problems, I would be loath to change it purely for stylistic reasons.
> AKA "if it ain't broken, don't fix it".

"decade" means "five years"? ;-)
Please have a look at the version log.

> It is almost always true that that there is some new language feature
> not used in code that has been around for a while.  (For any type of
> program, and any language.)  In general it is simply a waste of effort
> modifying existing code to use new language features if doing so will
> not immediately improve the performance or stability of the code.
> Neither of which seems to be the case here.

My OP was not about "some new language feature".
If anything, it will improve the readability of the code (in my 
opinion), which may inspire someone to further improve the code, or to 
use it somewhere else in the lib. Why do you think it will not improve 
the code? (certainly for stability/maintainability there is an obvious 
gain, perhaps you missed it in my earlier mails)


> Think about it, you can put a lot of work into rewriting 2geom, and when
> all is said and done, everything calling it will get exactly the same
> results at the same speed as they do now.  So why bother?

If none of my other arguments make sense, then please make do with my last:
mental entertainment.

Cheers,
   Johan

Re: [Lib2geom-devel] C-arrays

[mathog <ma...@ca...>]

On 06-Nov-2013 08:39, Nathan Hurst wrote:

> But that code has been used for a decade now with no evidence of
> problems, I would be loath to change it purely for stylistic reasons.

AKA "if it ain't broken, don't fix it".

It is almost always true that that there is some new language feature 
not used in code that has been around for a while.  (For any type of 
program, and any language.)  In general it is simply a waste of effort 
modifying existing code to use new language features if doing so will 
not immediately improve the performance or stability of the code.  
Neither of which seems to be the case here.

Think about it, you can put a lot of work into rewriting 2geom, and when 
all is said and done, everything calling it will get exactly the same 
results at the same speed as they do now.  So why bother?

Regards,

David Mathog
ma...@ca...
Manager, Sequence Analysis Facility, Biology Division, Caltech