Mail Archives: geda-user/2015/07/08/04:23:41
Hi Peter,
sorry for the delay, I've been distracted by other things.
On Sat, Jun 27, 2015 at 11:12:59AM +0100, Peter C.J. Clifton wrote:
> On 2015-06-26 00:30, Gabriel Paubert (paubert AT iram DOT es) wrote:
> >I've not changed any internal API at all yet. For now the patches in
> >this area boil down to these modifications in the structure
> >definitions:
> >
> >
> >diff --git a/src/polyarea.h b/src/polyarea.h
> >index 43fd93d..f63e83d 100644
> >--- a/src/polyarea.h
> >+++ b/src/polyarea.h
> >@@ -82,8 +82,8 @@ struct VNODE
> > {
> > VNODE *next, *prev, *shared;
> > struct {
> >- unsigned int status:3;
> >- unsigned int mark:1;
> >+ unsigned char status;
> >+ unsigned char mark;
> > } Flags;
> > CVCList *cvc_prev;
> > CVCList *cvc_next;
> >@@ -96,15 +96,15 @@ struct PLINE
> > Coord xmin, ymin, xmax, ymax;
> > PLINE *next;
> > VNODE head;
> >- unsigned int Count;
> > double area;
> > rtree_t *tree;
> >- bool is_round;
> > Coord cx, cy;
> > Coord radius;
> >+ unsigned int Count;
> >+ bool is_round;
> > struct {
> >- unsigned int status:3;
> >- unsigned int orient:1;
> >+ unsigned char status;
> >+ unsigned char orient;
> > } Flags;
> > };
> >
> >The reasons are:
> >- when the structure size is at least a multiple of 4 bytes (32 bit),
> > and more likely 8 (64 bit machines and some 32 bit because there is
> > a double in there), it is stupid to use 2 bit fields when two
> > unsigned char fields will do (extracting from bit fields and
> > especially assigning to is more expensive).
>
> Granted.. bitfields are ugly here. I didn't realise we used them.
>
> >- the is_round field in the middle of PLINE is very wasteful for
> > alignment.
>
> I put that there.. next to the coordinates (to be close to the
> variables it makes sense with).
I understand, but in this case, put all the other fields which
add up to the alignment constraints also, they won't waste memory.
The coordinates are probably the most frequenctly accessed fields
(maybe I'me wrong) and in any case filling alignment holes is
unlikely to hurt.
The structure declaration should group Count, Flags, and is_round
together, perhaps making the last one a char to make sure that is is not
32 bit on some ABIs. Count has to be first or last, but the whole group
may indeed be better put close to the coordinates.
>
> Great to move these, and with our piecewise linear circular circular
> approximations, we do get a lot of VNODEs.
Indeed, but what would it cost to have circular arc primitives in the
polygon boundaries (much fewer VNODES, and more complex algorithms)?
This said, when you have lots of items of a given type, standard memory
allocators may become a performance concern (and fortunately we're not
speaking of mutithreaded code here). In this case a magazine of reusable
items.
> I would, however, be surprised if this makes any measurable
> difference in speed, but I might be wrong.
I don't think it will make any, and it it does, it will probably be
indirectly through cache footprint and maybe avoiding bit-field
insertions.
>
> >I'm not sure for the area field , IIRC the alignment of doubles depends
> >on the ABI for 32 bit architectures and anyway 64 bit is what
> >really matters
> >these days.
>
> Agreed.
>
>
> Let me know if your shorted sliver cases is fixed by any
> (combination) of my patches. I suspect they will.
>
It is fixed by the first patch. Before applying your patch, I thought it
was some initialization problem because the sliver disappeared as soon
as I changed anything (pad or line size, etc) and then reverted the change.
For your second patch, I had started in a different direction: namely to
adapt the callers of frac_circle so that they work according to the
documentation: frac_circle draws all the points except the last. I'm not
sure that this is the best API, but it is what is documented (in general
I find the number of comments which obviously don't match the code really
appalling, I'm speaking only of the polygon code).
There are things in the code that don't make much sense, for example
the code for contour creation (poly_NewContour) seems to support the
creation of an initial empty contour, but it actually inserts a node
at (0,0). So nobody actually calls it with the parameter set to NULL
and the check is useless. Crashing would be better IMHO!
So should we allow empty contours of not? I'm undecided, although in
this case my tendency would be to say: in doubt do as ps/pdf/cairo
(which allow empty contours) but this may be overkill.
Another interface I don't like is frac_circle, as far as I can tell, it
is the only interface with which you can create both open (quarter or
half circles) contours and closed ones (full circles). I believe that
trying to conflate these two fundamentally different types of contour
creation functionality is the main issue here.
Regards,
Gabriel
- Raw text -