Re: [Kicad-developers] dyn_cast
I'm just going to rename it. I think the optimization was probably unnecessary, but if it works fine, the only thing it hurts is maintainability, which can be fixed somewhat by naming it something more obvious. No need to risk introducing bugs unnecessarily. Of course personally I'd like to see the parallel type system in EDA_ITEM disappear entirely, as duplication of information is always error-prone... but no need to remove this outside of an attempt to purge that. ;) On Wed, Apr 13, 2016 at 04:21:56PM -0400, Wayne Stambaugh wrote: > Please proceed with caution. I don't have any issue with renaming it to > make the intent clear but thorough real world testing should be done > before changing over to dynamic_cast<>. > > On 4/13/2016 12:42 PM, Tomasz Wlostowski wrote: > > On 13.04.2016 18:38, Chris Pavlina wrote: > >> I would argue - quite strongly! - that it doesn't matter if it's faster in > >> an > >> isolated test if it isn't used enough to actually affect the overall code > >> speed. Was this ever profiled in situ? This sort of thing just causes > >> headaches > >> when people misunderstand the usage and subtleties of the more limited > >> "optimized" replacement. I find it hard to believe that we call > >> dynamic_cast > >> enough for it to be a performance issue. > > > > Perhaps you're right, I'm not going to argue here. > > > >> > >> Also - *please* let me rename it. dynamic_cast vs dyn_cast is almost the > >> _textboox_ example of poor naming. It's absolutely 100% non-obvious to > >> someone > >> reading code using the latter _why_ it was chosen, what its advantage is > >> over > >> the normal one, and whether it has any subtle issues that can cause bugs. > >> At > >> least it should be called something like dynamic_cast_fast so there's > >> justification for its use in the actual code using it. > > > > Go on with the renaming. > > > > Tom > > > >> > >> On Wed, Apr 13, 2016 at 06:34:28PM +0200, Tomasz Wlostowski wrote: > >>> On 13.04.2016 18:19, Simon Richter wrote: > Hi, > > On 13.04.2016 18:13, Chris Pavlina wrote: > > > What is the purpose of dyn_cast<> in include/core/typeinfo.h? Why don't > > we just > > use dynamic_cast<>? And can we either replace the former with the > > latter, or > > add a comment to the former explaining its purpose? > > It uses the parallel type system in EDA_ITEM rather than RTTI, so it > works if RTTI is broken, e.g. when compiling with gcc 2.95. > > >>> > >>> I wrote it inspired with Clang/LLVM design which uses a very similar > >>> pattern. Sorry Simon, I didn't consider compatibility with gcc 2.95 > >>> would be of an advantage. My reasons were: > >>> > >>> 1) Much faster (code in the attachment): > >>> - dynamic_cast<> : 9090437 usecs > >>> - dyn_cast<> : 1832433 usecs (5x improvement) > >>> > >>> 2) Lightweight & compatible with existing Kicad type system. > >>> > >>> Cheers, > >>> Tom > >> > > > > > > ___ > > Mailing list: https://launchpad.net/~kicad-developers > > Post to : kicad-developers@lists.launchpad.net > > Unsubscribe : https://launchpad.net/~kicad-developers > > More help : https://help.launchpad.net/ListHelp > > > > ___ > Mailing list: https://launchpad.net/~kicad-developers > Post to : kicad-developers@lists.launchpad.net > Unsubscribe : https://launchpad.net/~kicad-developers > More help : https://help.launchpad.net/ListHelp ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] dyn_cast
Please proceed with caution. I don't have any issue with renaming it to make the intent clear but thorough real world testing should be done before changing over to dynamic_cast<>. On 4/13/2016 12:42 PM, Tomasz Wlostowski wrote: > On 13.04.2016 18:38, Chris Pavlina wrote: >> I would argue - quite strongly! - that it doesn't matter if it's faster in an >> isolated test if it isn't used enough to actually affect the overall code >> speed. Was this ever profiled in situ? This sort of thing just causes >> headaches >> when people misunderstand the usage and subtleties of the more limited >> "optimized" replacement. I find it hard to believe that we call dynamic_cast >> enough for it to be a performance issue. > > Perhaps you're right, I'm not going to argue here. > >> >> Also - *please* let me rename it. dynamic_cast vs dyn_cast is almost the >> _textboox_ example of poor naming. It's absolutely 100% non-obvious to >> someone >> reading code using the latter _why_ it was chosen, what its advantage is over >> the normal one, and whether it has any subtle issues that can cause bugs. At >> least it should be called something like dynamic_cast_fast so there's >> justification for its use in the actual code using it. > > Go on with the renaming. > > Tom > >> >> On Wed, Apr 13, 2016 at 06:34:28PM +0200, Tomasz Wlostowski wrote: >>> On 13.04.2016 18:19, Simon Richter wrote: Hi, On 13.04.2016 18:13, Chris Pavlina wrote: > What is the purpose of dyn_cast<> in include/core/typeinfo.h? Why don't > we just > use dynamic_cast<>? And can we either replace the former with the latter, > or > add a comment to the former explaining its purpose? It uses the parallel type system in EDA_ITEM rather than RTTI, so it works if RTTI is broken, e.g. when compiling with gcc 2.95. >>> >>> I wrote it inspired with Clang/LLVM design which uses a very similar >>> pattern. Sorry Simon, I didn't consider compatibility with gcc 2.95 >>> would be of an advantage. My reasons were: >>> >>> 1) Much faster (code in the attachment): >>> - dynamic_cast<> : 9090437 usecs >>> - dyn_cast<> : 1832433 usecs (5x improvement) >>> >>> 2) Lightweight & compatible with existing Kicad type system. >>> >>> Cheers, >>> Tom >> > > > ___ > Mailing list: https://launchpad.net/~kicad-developers > Post to : kicad-developers@lists.launchpad.net > Unsubscribe : https://launchpad.net/~kicad-developers > More help : https://help.launchpad.net/ListHelp > ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] dyn_cast
On 13.04.2016 18:38, Chris Pavlina wrote: > I would argue - quite strongly! - that it doesn't matter if it's faster in an > isolated test if it isn't used enough to actually affect the overall code > speed. Was this ever profiled in situ? This sort of thing just causes > headaches > when people misunderstand the usage and subtleties of the more limited > "optimized" replacement. I find it hard to believe that we call dynamic_cast > enough for it to be a performance issue. Perhaps you're right, I'm not going to argue here. > > Also - *please* let me rename it. dynamic_cast vs dyn_cast is almost the > _textboox_ example of poor naming. It's absolutely 100% non-obvious to someone > reading code using the latter _why_ it was chosen, what its advantage is over > the normal one, and whether it has any subtle issues that can cause bugs. At > least it should be called something like dynamic_cast_fast so there's > justification for its use in the actual code using it. Go on with the renaming. Tom > > On Wed, Apr 13, 2016 at 06:34:28PM +0200, Tomasz Wlostowski wrote: >> On 13.04.2016 18:19, Simon Richter wrote: >>> Hi, >>> >>> On 13.04.2016 18:13, Chris Pavlina wrote: >>> What is the purpose of dyn_cast<> in include/core/typeinfo.h? Why don't we just use dynamic_cast<>? And can we either replace the former with the latter, or add a comment to the former explaining its purpose? >>> >>> It uses the parallel type system in EDA_ITEM rather than RTTI, so it >>> works if RTTI is broken, e.g. when compiling with gcc 2.95. >>> >> >> I wrote it inspired with Clang/LLVM design which uses a very similar >> pattern. Sorry Simon, I didn't consider compatibility with gcc 2.95 >> would be of an advantage. My reasons were: >> >> 1) Much faster (code in the attachment): >> - dynamic_cast<> : 9090437 usecs >> - dyn_cast<> : 1832433 usecs (5x improvement) >> >> 2) Lightweight & compatible with existing Kicad type system. >> >> Cheers, >> Tom > ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] dyn_cast
I would argue - quite strongly! - that it doesn't matter if it's faster in an isolated test if it isn't used enough to actually affect the overall code speed. Was this ever profiled in situ? This sort of thing just causes headaches when people misunderstand the usage and subtleties of the more limited "optimized" replacement. I find it hard to believe that we call dynamic_cast enough for it to be a performance issue. Also - *please* let me rename it. dynamic_cast vs dyn_cast is almost the _textboox_ example of poor naming. It's absolutely 100% non-obvious to someone reading code using the latter _why_ it was chosen, what its advantage is over the normal one, and whether it has any subtle issues that can cause bugs. At least it should be called something like dynamic_cast_fast so there's justification for its use in the actual code using it. On Wed, Apr 13, 2016 at 06:34:28PM +0200, Tomasz Wlostowski wrote: > On 13.04.2016 18:19, Simon Richter wrote: > > Hi, > > > > On 13.04.2016 18:13, Chris Pavlina wrote: > > > >> What is the purpose of dyn_cast<> in include/core/typeinfo.h? Why don't we > >> just > >> use dynamic_cast<>? And can we either replace the former with the latter, > >> or > >> add a comment to the former explaining its purpose? > > > > It uses the parallel type system in EDA_ITEM rather than RTTI, so it > > works if RTTI is broken, e.g. when compiling with gcc 2.95. > > > > I wrote it inspired with Clang/LLVM design which uses a very similar > pattern. Sorry Simon, I didn't consider compatibility with gcc 2.95 > would be of an advantage. My reasons were: > > 1) Much faster (code in the attachment): > - dynamic_cast<> : 9090437 usecs > - dyn_cast<> : 1832433 usecs (5x improvement) > > 2) Lightweight & compatible with existing Kicad type system. > > Cheers, > Tom ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] dyn_cast
On 13.04.2016 18:19, Simon Richter wrote:
> Hi,
>
> On 13.04.2016 18:13, Chris Pavlina wrote:
>
>> What is the purpose of dyn_cast<> in include/core/typeinfo.h? Why don't we
>> just
>> use dynamic_cast<>? And can we either replace the former with the latter, or
>> add a comment to the former explaining its purpose?
>
> It uses the parallel type system in EDA_ITEM rather than RTTI, so it
> works if RTTI is broken, e.g. when compiling with gcc 2.95.
>
I wrote it inspired with Clang/LLVM design which uses a very similar
pattern. Sorry Simon, I didn't consider compatibility with gcc 2.95
would be of an advantage. My reasons were:
1) Much faster (code in the attachment):
- dynamic_cast<> : 9090437 usecs
- dyn_cast<> : 1832433 usecs (5x improvement)
2) Lightweight & compatible with existing Kicad type system.
Cheers,
Tom
#include
#include "profile.h"
#include "typeinfo.h"
const int n_iter = 10;
const int id_base = 0;
const int id_derived = 1;
class Base {
public:
Base()
{
m_type = id_base;
}
virtual ~Base() {};
static inline bool ClassOf( const Base* aItem )
{
return aItem && id_base == aItem->Type();
}
int Type() const
{
return m_type;
}
protected:
int m_type;
int m_count;
};
class Derived : public Base {
public:
static inline bool ClassOf( const Base* aItem )
{
return aItem && id_derived == aItem->Type();
}
Derived()
{
m_type = id_derived;
}
void DoSomething()
{
m_count++;
}
};
main()
{
Base *b = new Derived();
prof_counter cnt;
prof_start(&cnt);
for(int i = 0; i (b);
d->DoSomething();
}
prof_end(&cnt);
printf("dynamic_cast<> : %d usecs\n", cnt.usecs());
prof_start(&cnt);
for(int i = 0; i (b);
d->DoSomething();
}
prof_end(&cnt);
printf("dyn_cast<> : %d usecs\n", cnt.usecs());
}
___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] dyn_cast
Hi, On 13.04.2016 18:13, Chris Pavlina wrote: > What is the purpose of dyn_cast<> in include/core/typeinfo.h? Why don't we > just > use dynamic_cast<>? And can we either replace the former with the latter, or > add a comment to the former explaining its purpose? It uses the parallel type system in EDA_ITEM rather than RTTI, so it works if RTTI is broken, e.g. when compiling with gcc 2.95. I believe this should go. Simon signature.asc Description: OpenPGP digital signature ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] dyn_cast and ClassOf
On Mon, Jun 09, 2014 at 11:10:52AM -0500, Dick Hollenbeck wrote: > We're in complete agreement then, let's switch to Cobol. Sorry, that would be enough only if kicad did stock reports :D (atleast probably the builtin reports would come out neatly formatted, since wx still doesn't handle correctly multibyte sequences...) -- Lorenzo Marcantonio Logos Srl ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] dyn_cast and ClassOf
We're in complete agreement then, let's switch to Cobol. ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] dyn_cast and ClassOf
On Mon, Jun 09, 2014 at 08:55:57AM -0500, Dick Hollenbeck wrote: > On that good day the switch() code generater will use a jump table with > machine code which > could also come from this type of construct: > > (*jump_table[switch_enum - switch_lowest_value])() On other *very* good days the compiler snuff out the ifs and do a jump table (seems incredible but it does...). However even the venerable borland 2 compiler did the range optimization you said. > If gaps in the enum are not overwhelming, then those will get filled with > jumps to > "default:". The main point is that if this is in a drawing loop, then it > becomes a > hotspot. Not having multiple else if tests makes it very lean. There are no > tests, only > one indexed lookup. You could force that path with a lookup table to functions, indexed by the type. Usually the compiler is smart enough to recognize it anyway (except fujitsu ones, where I had to do the table trick) > We moved PAINTER out of the object hierarchy for good reasons. Otherwise it would been have simply a call to [VTBL+offset]. Breaks all the OO advantages but I presume the reasons where good. Also I don't particularly care about OO practices, unless the gain is obvious or forced by management. To run a stock reports we still use COBOL, and there is a reason for that:P > Unless you can match the kind of performance that a large switch() provides, > I think this > ClassOf is not a good path to entertain. I wasn't thinking particularly about the painter logic; it seemed to me a somewhat convolved way to do a simple thing. However it seems that in 'modern' C++ you always have to do convolved things for simple tasks... > Nor does it fix anything. It's faster than dynamic_cast (certainly, since it's not solving the generic IS-A problem) and *maybe* slightly more convenient than an 'if (type ==) static_cast'. Anyway: I don't thing that would be such an hotspot in the paint code, relatively talking. Just sum out all the math needed for building the opengl points and stuff and you'll see probably the cost for dispatching on the kind of object is swamped by the actual painting work. IMHO the switch would be the most simple way to do the dispatch (until proven otherwise, the KISS principle rules). Further profiling could show the actual hotspots (either gprof or valgrind). -- Lorenzo Marcantonio Logos Srl ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] dyn_cast and ClassOf
On 06/06/2014 03:22 PM, Lorenzo Marcantonio wrote:
> On Fri, Jun 06, 2014 at 08:30:59PM +0200, Tomasz Wlostowski wrote:
>
>> Either use only C++ RTTI or a type ID field. Mixing both is ugly.
>
> Or better yet trust your interfaces:P if you need to know the kind of
> object you have, then there is a design issue, usually. Also type ID
> doesn't handle inheritance easily, which is bad.
>
>> This means a pretty much complete rewrite of Kicad...
>
> Of course. That was meant for *new* code.
>
>> I was reading Clang/LLVM source code and found this dyn_cast/isa pattern
>> very elegant and lightweight. It's a mine of practical OO design ideas.
>
> I could argue about the elegance of the whole template cruft:P it seems
> that in 'modern' C++ you need to write more boilerplate/magical
> definition than actual 'work' code.
>
>> - it's much faster than C++ dynamic_cast,
>
> Same analysis I did.
>
>> - you don't have to remember the type ID <> class association (e.g.
>> PCB_LINE_T vs class DRAWSEGMENT)
>
> Have you looked for using typeid? could be an interesting alternative
> (altough I don't seen any obvious advantage over the kicad type tag,
> other than being a standard construct)
>
>> - can be implemented without disrupting the code
>
> Other than all these ugly things around :D
>
>> - saves a line (or three lines) of code on every down-cast:
>
> Agree with that. Still I found that usually you need to check the type
> before the downcast anyway, so you have to do the if statement... also
> the downcasted pointer has reduced scope which is IMHO a big advantage.
>
> The common pattern I've found is:
>
> if (stuff->type is CLASS_T) {
> CLASS *ptr = static_cast(stuff);
> // do things with ptr
> }
>
> with your solution the pattern would be:
>
> CLASS *ptr = dyn_cast(stuff); // More or less
> if (ptr) {
> // do thing with ptr
> }
>
> When you expand the template the resulting code is pretty much the same:
>
> CLASS *ptr = (stuff->type is CLASS_T) ? static_cast(stuff) : NULL;
> if (ptr) {
> // do thing with ptr
> }
>
> In the end it's down to individual preference, almost like indentation
> conventions...
>
> For the other people however I'd add a big comment explaining how it
> works and the limitations (i.e. you need to define the correct ClassOf
> statics and maybe other things). I guess it *could* handle inheritance,
> putting the closure of the derived classes in the parent (so that an
> hypotetical EDA_ITEM::ClassOf would contain almost all of the class
> tags:P)
>
> I've had bad experiences wrestling with the TRACK/VIA/SEGZONE class
> branch virtuals...
>
In PAINTER and in a few other places there are dispatches using a switch(). A
switch(),
on a good day, can generate much more efficient code than a series of else
if()s.
On that good day the switch() code generater will use a jump table with machine
code which
could also come from this type of construct:
(*jump_table[switch_enum - switch_lowest_value])()
If gaps in the enum are not overwhelming, then those will get filled with jumps
to
"default:". The main point is that if this is in a drawing loop, then it
becomes a
hotspot. Not having multiple else if tests makes it very lean. There are no
tests, only
one indexed lookup.
We moved PAINTER out of the object hierarchy for good reasons.
Unless you can match the kind of performance that a large switch() provides, I
think this
ClassOf is not a good path to entertain.
Nor does it fix anything.
Dick
___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] dyn_cast and ClassOf
On Fri, Jun 06, 2014 at 08:30:59PM +0200, Tomasz Wlostowski wrote:
> Either use only C++ RTTI or a type ID field. Mixing both is ugly.
Or better yet trust your interfaces:P if you need to know the kind of
object you have, then there is a design issue, usually. Also type ID
doesn't handle inheritance easily, which is bad.
> This means a pretty much complete rewrite of Kicad...
Of course. That was meant for *new* code.
> I was reading Clang/LLVM source code and found this dyn_cast/isa pattern
> very elegant and lightweight. It's a mine of practical OO design ideas.
I could argue about the elegance of the whole template cruft:P it seems
that in 'modern' C++ you need to write more boilerplate/magical
definition than actual 'work' code.
> - it's much faster than C++ dynamic_cast,
Same analysis I did.
> - you don't have to remember the type ID <> class association (e.g.
> PCB_LINE_T vs class DRAWSEGMENT)
Have you looked for using typeid? could be an interesting alternative
(altough I don't seen any obvious advantage over the kicad type tag,
other than being a standard construct)
> - can be implemented without disrupting the code
Other than all these ugly things around :D
> - saves a line (or three lines) of code on every down-cast:
Agree with that. Still I found that usually you need to check the type
before the downcast anyway, so you have to do the if statement... also
the downcasted pointer has reduced scope which is IMHO a big advantage.
The common pattern I've found is:
if (stuff->type is CLASS_T) {
CLASS *ptr = static_cast(stuff);
// do things with ptr
}
with your solution the pattern would be:
CLASS *ptr = dyn_cast(stuff); // More or less
if (ptr) {
// do thing with ptr
}
When you expand the template the resulting code is pretty much the same:
CLASS *ptr = (stuff->type is CLASS_T) ? static_cast(stuff) : NULL;
if (ptr) {
// do thing with ptr
}
In the end it's down to individual preference, almost like indentation
conventions...
For the other people however I'd add a big comment explaining how it
works and the limitations (i.e. you need to define the correct ClassOf
statics and maybe other things). I guess it *could* handle inheritance,
putting the closure of the derived classes in the parent (so that an
hypotetical EDA_ITEM::ClassOf would contain almost all of the class
tags:P)
I've had bad experiences wrestling with the TRACK/VIA/SEGZONE class
branch virtuals...
--
Lorenzo Marcantonio
Logos Srl
___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] dyn_cast and ClassOf
On 06.06.2014 19:33, Lorenzo Marcantonio wrote:
Why reinventing the wheel? AFAIK C++ has a pretty good RTTI...
Doing it by hand seems to me quite error prone and distracting (and
I don't want to know what happens when subclassing). Had to do that in
the old borland C++ days (no templates, Borland intrusive containers),
didn't like it:D
Let's not get back to these dark, ancient times.
Either use only C++ RTTI or a type ID field. Mixing both is ugly.
In fact good design practices (at least in OO) would condemn the whole
KICAD_T enum :P
I think that instead of using such kind of expedient better (virtual)
interfaces (in the java sense, pure abstracts to be implemented) should
be designed with a refactoring.
This means a pretty much complete rewrite of Kicad...
That is, if you want to use C++ in the OO way (in imperative style I'd
simply have checked the type member, without all that template fluff).
I was reading Clang/LLVM source code and found this dyn_cast/isa pattern
very elegant and lightweight. It's a mine of practical OO design ideas.
This template stuff has some advantages:
- it's much faster than C++ dynamic_cast,
- you don't have to remember the type ID <> class association (e.g.
PCB_LINE_T vs class DRAWSEGMENT)
- can be implemented without disrupting the code
- saves a line (or three lines) of code on every down-cast:
Instead of:
if(item->Type() == something)
{
DERIVED *x = static_cast item;
single-line-action
}
all you need is:
if ( SOMETHING *x = dyn_cast(item) )
single-line-action
Regards,
Tom
___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help : https://help.launchpad.net/ListHelp
