Feedback on 7.0 RTTI mechanism implementation

Forums: 

In current master (7.0 preview), DEFINE_STANDARD_RTTI macro includes declaration and inlined definition of the static
and virtual method as follows (see Standard_Type.hxx):
static const char* get_type_name () { return #Class; } \
virtual const Handle(Standard_Type)& DynamicType() const { return STANDARD_TYPE(Class); }

At the same time previously used macro IMPLEMENT_STANDARD_RTTIEXT is defined empty (see Standard_DefineHandle.hxx).

To ensure creation of a static variable Handle_Standard_Type for each type, a method opencascade::type_instance::get() contains an explicit use of the variable myInstance what makes compiler to ensure its presence.

AFAIU, this creates the following downsides:
1. Use of inlined virtual methods would enforce the linker create duplicated implementation in each shared library.
2. Likewise for inlined static method returning a string constant.
3. No way to find a moment where the respective static variable of Handle_Standard_Type is created (as all instances are created by dynamic initializers during library loading). In some cases this can be helpful (for instance, during debugging of #26913).

Instead, why not to create (or rather restore) a more explicit and streamlined approach (used in earlier versions):
- make out of line implementations of get_type_name() and DynamicType();
- create explicit definitions of static variables defining each type.

All this can be put (back) into IMPLEMENT_STANDARD_RTTIEXT macros to be used in .cxx.

Any thoughts on this ?

Best regards,
Roman

Roman Lygin's picture

As we move further in porting to 7.0, another issue popped up which is creating of redundant dependencies from user libraries to OCC's.

Here is a use case:

User's library A depends on OCC library TKLCAF (uses TDocStd_Document); the A's link list includes TKLCAF.

TDocStd_Document.hxx (which includes CDM_Document.hxx) now injects inlined code of CDM_Document::DynamicType() and hence Standard_Type::Instance into A. When linking A on Linux the linker complains about unresolved symbols defined in TKCDM. Same for Geom2d and other libraries - see below. Thus, this requires addition of TKCDM and TKG2d into the link list of A which previously was not necessary.

Thus, due to inlining, the definitions become viral and penetrate user libraries which do not need them. This increases porting overhead, contaminates dependency list and complicates maintenance (as you now don't clearly know what you really depend on and what comes as derivation).

../lin64/gcc4/obj/QOCoreLib/Debug/QOBase_Action.o: In function `opencascade::type_instance::get()':
/Dev/occ/git.dev/fix-lin64-gcc4-debug/inc/Standard_Type.hxx:171: undefined reference to `typeinfo for Geom2d_Curve'
../lin64/gcc4/obj/QOCoreLib/Debug/QOBase_Action.o: In function `opencascade::type_instance::get()':
/Dev/occ/git.dev/fix-lin64-gcc4-debug/inc/Standard_Type.hxx:171: undefined reference to `typeinfo for CDM_Document'

Mikael Aronsson's picture

Hi !

I would also be interested in the example

Mikael mikael_aronsson@mail.bip.net

Terrence Mitchem's picture

I would also be interested in receiving information about this.

Kai Anding's picture

Dear Sylvain!

I would also be very interested in this example VC++ project file. Many thanks in advance!

Kai

Alan Hazell's picture

Sylvain,

I too would also find your Visual project example extremely useful as I have limited knowledge of compiling at present.

Many thanks,

Alan Hazell Loughborough University

Mikael Aronsson's picture

Hi !

I must be doing something wrong here, I am trying to compile a debug version of TKCAF, and I have put together a project to compile all the source files, but I get unresolved external errors, many of them, as it look I think it is because most publics are declared with declspec(import) by default, which is good for an application using the library, but the library should be compiled with declspec(export).

__Standard_DLL controls this, wright ?

What I don't get is that this means that all publics are declared either as import or export, compiling for example TKCAF would require the the stuff for that library should be declared with export and the rest as import, am I completley wrong here or... ?

I looked in the example project but I could not find anything special in there, and that does not declare the __Standard_DLL.

I guess I am doing something very stupid, but I don't now what it is :)

Mikael

Mikael Aronsson's picture

Hi !

Sorry, as usual it's me being stupid, I completley missed the fact that there are source files under the drv dubdirectory to, I though that was only include files, after adding them to the project to everything (almost) compiled as it should.

I did get an unresolved external on alloc(), but this is available on Windows to, but it is named _alloc(), I renamed it in the source file and then it compiled ok, I do think you can do the same thing by linking to oldnames.lib, but I didn't try that.

Mikael

Msaaf Omar's picture

in your example. Thanks in advance

Sylvain Lecluze's picture

Hello, I would be intersted too Thanks Sylvain

Peggy Rocher's picture

I have troubles when compiling the TKService module with VC++: There are errors with the two include files unistd.h (in Xw_Extension.h) and X11/Xlib.h (in ImageUtility_X11Display.hxx), but I think there is no need for these files because I build it for NT.

Which settings should I use not to have these problems any more ?

Jean Rahuel's picture

You does not need Xw on NT. Xw is used only on UNIX.

Best Regards,

Jean Rahuel

Philippe Centa's picture

As far as I remember, when you build for windozz (98, NT or whatever) even though VC++ should tell you with /D "WIN32" , you have to add /D "WNT" in your 'define' settings to reinfoce your position ! (my suggestion is to add #ifdef WIN32 define WNT somewhere)

Regards,

Philippe Centa

Peggy Rocher's picture

The solution for this problem is not to add the Xw, Xdps and ImageUtility packages to the project (TKService) you want to compile ...

Mikael Aronsson's picture

Hi !

Do you have the #define WNT in there, anyway I don't tink you should even compile these files at all on VC++, these are unix files only I guess.

Mikael

Andrey BETENEV's picture

Hello Roman,

First, thank you for indicating the problem and suggesting a way to address it.

I have registered an issue #26936 for this problem. Please try the fix available in branch CR26936. As discussed before, that fix replaces macro DEFINE_STANDARD_RTTI by two -- DEFINE_STANDARD_RTTI_INLINE and DEFINE_STANDARD_RTTIEXT. The latter should be used together with IMPLEMENT_STANDARD_RTTIEXT, and together they should have essentially the same features as in OCCT 6.x.

The result is considerable reduction of size of OCCT binaries -- by 8% for VC++ 10, 15% for VC++ 14 and CLang 3.6.2, and 25% for GCC 5.2. The effect should be stronger on dependent projects, please share your numbers.

Note that this does not yet make binaries of OCCT 7 less than that of OCCT 6.9.1 (except for VC++ 10), they still are ~15% greater for modern compilers. I suspect that this can be mostly due to multiple instances of NCollection templates, and will try to address this later.

Andrey

Roman Lygin's picture

Hi Andrey,

Thank you very much for paying attention to the issue and willingness to address it.
I have rebuilt our product with three versions of OCC to track size dynamics. Data are here - I cannot figure out how to attach here on the forum:-\.

General observations:
1. Release mode sizes are generally on par, within [-9%,+5%]
2. Debug mode sizes were [+12%,+64%] on master of Nov 22 and [+5%,+17%] on CR26396. Reduction of CR26396 vs master of Nov 22 - [-29%,-7%].
So CR26396 definitively helps.

3. Macro definitions DEFINE_STANDARD_RTTI_INLINE are used in templates/generics (e.g in NCOLLECTION_HARRAY1, etc). That causes compilation error on user-defined types (due to former use of IMPLEMENT_STANDARD_RTTIEXT in .cxx) but that is minor issue, resolvable with #if OCC_VERSION_HEX < 0x070000.

I have not tested yet the use of the registry in Standard_Type.cxx but assume duplicated type registrations are now significantly reduced, what should also improve loading time.

So overall must be good. Thanks again for this effort.
Roman

Andrey BETENEV's picture

Hello Roman,

Thank you for feedback! This change has been raised to master last Friday.

Andrey

Roman Lygin's picture

Cool! Thanks Andrey.