two patches

[Dirk Hohndel]

On Fri, Dec 12, 2014 at 05:03:54PM -0200, Tomaz Canabrava wrote:
> > So I'm puzzled how often we are adding bookmark events and how that
> > affects performance... or is this really just a first of a series and the
> > others are more performance relevant?
> 
> First of a series.

OK

> > Dear C expert... I am curious. Why are you doing this with a struct event
> > **
> > instead of just a struct event *... you need the ** if you want to be able
> > to modify the list in an elegant way, but all you do here is walk the
> > list, so this could drop one level of indirection... or am I missing
> > something?
> 
> Copied from the code just above that one.

But that code modified the list, right?

> > And why add the curly braces, anyway?
> 
> I was debugging and forgot to remove them.

Tsktsktsk. So you added something because you forgot to remove it and
introduced whitespace damage that way...

> > This could do something really bad if for some reason
> > internalEvent->time.seconds is negative, right? then entry[0].sec (which
> > should always be 0) could already be bigger and we access element entry[-1]
> 
> Is that possible?

Off the top of my head I'd say "unlikely", but I worry about dives
imported from other sources or something else that could break this
assumption. So I'd rather have you test for i > 0 before you access
entry[i - 1]

> > So this is pretty invasive and the patch has a few issues.
> >
> > Why should this go in before 4.3? Is there a specific bug this addresses?
> >
> 
> I was trying to find out why when moving a node for a few seconds while
> adding a new dive used 100% of the CPU in release mode. turned out it was
> because of too many replots that used the *very* expensive calls to
> PainterPath.
> while I was trying to fix that I got carried away.

I hear that happens. So does this mean "I'll rework this for until after
4.3" or "I'll rework this to address the issues mentioned and then submit
the series that fixes the bigger problem later today"?

> I think I'll just
> generate the pixmap of the Glyphs and reuse them in the future for the
> PainterPath issue.

Or that and we forget the patch? That seems wrong because the idea in the
patch seems correct.

/D