Remove allocation when comparing type info objects #370

Ingrater · 2012-12-24T11:35:57Z

As request by Andrei Alexandrescu on the newsgroup some time ago, here a pull request for my changes to type info comparison. It has several advantages over the current approach:

-Fixes next() for TypeInfo_Const, TypeInfo_Typedef and TypeInfo_Vector
-Comparing two type info objects will no longer allocate two strings and compare them. This is one step needed to get a non leaking d-runtime when working without a GC.
-Variadic functions can be implemented more easily and efficiently. For a example see: https://github.com/Ingrater/thBase/blob/master/src/thBase/format.d#L257
-It is a lot easier to unqualify type info objects across dll boundaries. For a example see https://github.com/Ingrater/thBase/blob/master/src/thBase/format.d#L13

-Comparing two TypeInfo objects no longer allocates and compares strings

alexrp · 2012-12-24T11:39:23Z

src/object_.d

+            switch(lhsType)
+            {
+                case TypeInfo.Type.Struct:
+                    {


Please dedent one level (and you probably don't need those braces either).

Dedent what exactl? The entire case blocks?

Yes. Usually we just leave out the braces and indent the code within by one level.

Yuck. Really? I'd argue that the cases need to be indented. It's the fact that the brace was further indented was the problem. I'd never not indent the cases and am not aware of anywhere in Phobos which doesn't (though it may be in there somewhere). And there's nothing wrong with using braces inside the case statements. I usually do and think that it's cleaner if you do (though unlike in C, it's not actually needed for scoping). They just need to line up with the case statements.

My point is this is right:

switch (foo) { case bar: baz(); }

Or this, if you really want:

switch (foo) { case bar: { baz(); } }

But not this:

switch (foo) { case bar: { baz(); } }

That, I agree with, but that means that he needs to change it again, because he misunderstood you and made the cases line up with the switch.

Oh, I see. Yeah, that's not what I meant. :)

Weird. I've always seen cases outdented because they're labels. Indenting the makes the flow less clear IMO. I think outdenting is consistent with the classic style layouts as well. But as long as the code is consistently formatted I'm not going to quibble about spacing.

alexrp · 2012-12-27T15:57:05Z

src/object.di

@@ -67,6 +67,51 @@ struct OffsetTypeInfo

 class TypeInfo
 {
+    enum Type 
+    {
+        Info,


The convention is lower case for enum members.

For names that conflict with keywords, we append a _.

Do we rellay want this in this case? Almost all of these are keywords and would end up with a underscore at the end. Will look quite ugly when using these in code.

Yes. That's what we want. That's the coding style. It's what we've done elsewhere when we've needed symbol names which match keywords (e.g. std.traits does it).

Not to mention, most code won't be using these anyway, so even if you think they're a bit ugly, they'll affect a rather small number of programs. Regardless though, it is the agreed upon style for enums.

yes, camelCase and append underscores if necessary please

9rnsr · 2012-12-29T15:13:35Z

This change breaks Phobos unittests. Could you post a fix-up pull for Phobos?

Ingrater · 2012-12-29T15:31:16Z

I didn't have time to check what exactly breaks within the phobos unittests. But I will investigate and fix this, one way or the other.

alexrp · 2013-01-21T18:28:29Z

ping?

Ingrater · 2013-01-21T19:10:55Z

I'm having exams at the moment, this will have to wait for 3 weeks.

andralex · 2013-01-22T01:55:45Z

good luck!

Conflicts: src/object.di

…us other minor issues.

Ingrater · 2013-02-20T17:43:33Z

I have a question about some implementation detail. Please see and discuss: http://forum.dlang.org/thread/kg1pcr$197c$1@digitalmars.com

Ingrater · 2013-02-26T06:10:37Z

Anyone? Should I just choose whatever I find best?

alexrp · 2013-02-26T06:12:21Z

cc @andralex

WalterBright · 2013-03-22T00:32:51Z

This pull grafts tagged variants onto the TypeInfo, rather than use virtual functions. Why not use virtual functions? That also avoids the whole issue of that .next returns.

(I know that the compiler itself uses tagging sometimes! But I'm not sure that is a good idea there, either.)

Ingrater · 2013-03-22T19:30:16Z

Because I designed it with writing vararg functions in mind. With tagging you can write vararg functions by using switch case statements to decide what to do for a certain type.

MartinNowak · 2013-03-23T15:43:55Z

Because I designed it with writing vararg functions in mind. With tagging you can write vararg functions by using switch case statements to decide what to do for a certain type.

Sounds like you could as well compare the TypeInfo.classinfo ptr. Could you please provide a code example of what you intend to do here.

MartinNowak · 2013-03-23T15:53:25Z

src/object_.d

+                auto lhsTypedef = cast(const(TypeInfo_Typedef))cast(void*)lhs;
+                auto rhsTypedef = cast(const(TypeInfo_Typedef))cast(void*)rhs;
+                return lhsTypedef.name == rhsTypedef.name;
+            default:


How can you ensure that every unhandled Type has a valid TypeInfo.next field.
You should instead list all TypeInfos with a valid .next field explicitly.
It's also fairly confusing to read that TypeInfo.Type.double_ is handled by a recursive call with .next is null.

They don't need a valid next field. Next may return null which will terminate the (tail) recursion. If TypeInfo.next would not return a const TypeInfo the tail recursion could be replaced by a loop.
Sorry but I don't see a TypeInfo.Type.double_ in the code?

If TypeInfo.next would not return a const TypeInfo the tail recursion could be replaced by a loop.

This has been resolved with #359, please use a loop.

It's kind of bad that we have a const system that basically requires tail recursion in cases like this one (if you don't want to break the type system), and yet we are (for some reason??) allergic to recursion.

This is an isolated issue with const object references and there is Rebindable to work around it.

Whats the problem with tail recursion? D supports functional programming, so why not use it?
Also I'm going to wait until there is agreement that this fix is actually wanted before investing even more time into it which might be wasted.

Let's stop that discussion here. I'm basically nitpicking on the readability of this method but it's not a big deal.

Ingrater · 2013-03-25T19:39:00Z

No I can not compare the type info ptr because then it will not work across dll boundaries.
The description of the pull request says exactly what I'm trying to do here.

MartinNowak · 2013-03-25T20:23:54Z

No I can not compare the type info ptr because then it will not work across dll boundaries.

That's an ODR violation and does not need a fix in TypeInfo, see #142.

Ingrater · 2013-03-25T20:27:31Z

Well but until this ODR violation gets fixed a lot of time may pass.
Also the other two issues remain:

Comparing type info objects produces garbage (leaks memory)
Writing d-vararg functions is unneccessary complicated

MartinNowak · 2013-03-26T01:22:52Z

No I can not compare the type info ptr because then it will not work across dll boundaries.

OK, that's partly correct, e.g. for TypeInfo_Class of template classes.
Those are COMDAT but are unfortunately used in multiple DLLs/SOs unless we find a better solution.
The same is not true for basic types, those are strong symbols in druntime and we shouldn't simply hack up something that pretends working DLL support.

As it stands the best solution is to handle this differences in opEquals.
Because TypeInfo_Class can be COMDATs you additionally compare the names and baseclasses but you wouldn't for TypeInfo_i.
BTW I really dislike the idea of comparing names, because IMO it's perfectly valid to load an updated version of a class which is a distinct type with the same name.

Writing d-vararg functions is unneccessary complicated

What's the problem of replacing

switch (ti.type)
{
case TypeInfo.Type.UByte: format(va_arg!ubyte(argptr)); break;
case TypeInfo.Type.UShort: format(va_arg!ushort(argptr)); break;
case TypeInfo.Type.UInt: format(va_arg!uint(argptr)); break;
default: break;
}

with

if (ti == typeid(ubyte)) format(va_arg!ubyte(argptr));
else if (ti == typeid(ushort)) format(va_arg!ushort(argptr));
else if (ti == typeid(uint)) format(va_arg!uint(argptr));

?

Note that the virtual opEquals will correctly handle comparison.

MartinNowak · 2013-03-26T01:37:18Z

BTW I just thought we should create a TypeInfo_Template which is emitted together with a TypeInfo_Class/Struct/... into EVERY object filed using the template instantiation. It's only purpose would be to specialize opEquals for multiple instances.
All typeinfos for non-templated UDT would ONLY go into the object that defines that type.
The former ones should be COMDATs to reduce the binary size but latter ones should not as they must be unique.

WalterBright · 2013-03-26T03:04:10Z

I'm beginning to think that this would be much simpler if the test for equality was just a comparison of the pointers, and if the pointers are not equal, then the mangled type string is compared. The compiler can be adjusted to emit the mangled string for each typeinfo instance.

Ingrater · 2013-03-26T06:43:54Z

What's the problem of replacing

switch (ti.type)
{
case TypeInfo.Type.UByte: format(va_arg!ubyte(argptr)); break;
case TypeInfo.Type.UShort: format(va_arg!ushort(argptr)); break;
case TypeInfo.Type.UInt: format(va_arg!uint(argptr)); break;
default: break;
}

with

if (ti == typeid(ubyte)) format(va_arg!ubyte(argptr));
else if (ti == typeid(ushort)) format(va_arg!ushort(argptr));
else if (ti == typeid(uint)) format(va_arg!uint(argptr));

?

Your way has the complexity O(N). My way has O(1). Even std.format does not use your way. std.format partially parses the the mangeled type string and does a switch on that. Its also currently not easily possible to unqualify a type info object. If you find out that it is a const T, you can't get the T because calling next() will jump both the const and the T type info object. Just try implementeing a full printf wrapper with the current TypeInfo system. I'm my eyes its unneccessarily difficult to make it work for all types.

I'm beginning to think that this would be much simpler if the test for equality was just a comparison of the pointers, and if the pointers are not equal, then the mangled type string is compared. The compiler can be adjusted to emit the mangled string for each typeinfo instance.

But this would compare manageld type strings in most cases, because the common case is that the type info pointers don't match. With my implementation it will just compare two ints in most cases.

The same is not true for basic types, those are strong symbols in druntime and we shouldn't simply hack up something that pretends working DLL support.

AFAIK we already pretend working dll support?

BTW I really dislike the idea of comparing names, because IMO it's perfectly valid to load an updated version of a class which is a distinct type with the same name.

I disagree. I very recently wanted exactly that. I loaded a updated version of the class and wanted it to be the same type so I can make changes to code without restarting the application. See: http://3d.benjamin-thaut.de/?p=25

MartinNowak · 2013-03-26T13:33:21Z

I disagree. I very recently wanted exactly that. I loaded a updated version of the class and wanted it to be the same type so I can make changes to code without restarting the application. See: http://3d.benjamin-thaut.de/?p=25

I'll get more into the TypeInfo problems and we will find an appropriate solution but I'll have to defer this until next week.

MartinNowak · 2013-03-26T13:46:43Z

Your way has the complexity O(N). My way has O(1).

You can switch to template vararg functions for performance and I doubt that replacing N pointer comparisions with 1 table lookup would get you a much faster printf wrapper.

MartinNowak · 2013-04-08T03:16:15Z

I loaded a updated version of the class and wanted it to be the same type

Two distinct TypeInfo_Class instances with different vtables and init[] data should not compare equal.
I also don't see that your code relies on that fact.

MartinNowak · 2013-04-08T03:30:06Z

I'm beginning to think that this would be much simpler if the test for equality was just a comparison of the pointers, and if the pointers are not equal, then the mangled type string is compared. The compiler can be adjusted to emit the mangled string for each typeinfo instance.

In case that two shared libraries instantiate the same template class we get two weak TypeInfo instances.
Those get merged if at least one definition is available when linking the executable.
In the case that both shared libraries are loaded at runtime the weak symbols won't get merged and we need to resort to comparing the mangled name (also vtable size and init size must match, not their content though).

NB:
C++

MartinNowak · 2013-04-08T03:55:36Z

The compiler can be adjusted to emit the mangled string for each typeinfo instance.

That would potentially consume a lot of space. AFAIU we only need mangle comparison for weak TypeInfos. So we should use a dedicated wrapper that forwards all methods similar to TypeInfo_Typedef.

class TypeInfo_Weak : TypeInfo
{
    override bool opEquals(Object o)
    {
        if (this is o)
            return true;
        else if (o.classinfo is TypeInfo_Weak.classinfo)
        {
            auto c = *cast(const TypeInfo_Weak*)&o;
            return this.base == c.base &&
                this.mangledName == c.mangledName;
        }
        return false;
    }

    // other overrides like TypeInfo_Typedef

    TypeInfo base;
    string mangledName;
}

MartinNowak · 2013-04-08T04:08:28Z

I don't see a very convincing argument to add tagged classes. Most TypeInfo comparison are simple pointer comparisons and weak TypeInfo can get a mangledName field as fallback.

It's true that cross-DLL TypeInfo comparison currently fails as well as cross-DLL exception handling.
This is a consequence of linking DLLs against a static phobos/druntime thereby violating ODR. This is a
longstanding bug that gets solved by finalizing Windows DLL support.

andralex · 2013-05-06T01:35:56Z

@dawgfoto what's the status of this?

MartinNowak · 2013-05-06T02:10:56Z

I don't think faster vararg functions are a compelling enough use cases to make TypeInfo a tagged variant. For now replacing the tag comparisons with ti.classinfo == TypeInfo_Struct.classinfo should do the job.

Ingrater · 2013-05-08T14:48:50Z

I don't think faster vararg functions are a compelling enough use cases to make TypeInfo a tagged variant. For now replacing the tag comparisons with ti.classinfo == TypeInfo_Struct.classinfo should do the job.

This will still break in case of dlls until a shared druntime is implemented (which will not happen on all plattforms in the near future in my opinion). But if you don't think this is worth it I don't care anymore.

MartinNowak · 2013-05-08T18:13:31Z

I opened a Bugzilla 10048 for the issue.

This will still break in case of dlls until a shared druntime is implemented

Because it uses == instead of is, it should work, as long as we account for it in TypeInfo_Class.opEquals.

Ingrater added 2 commits December 24, 2012 12:24

-Added type property to all TypeInfo classes.

c3d6ec4

-Comparing two TypeInfo objects no longer allocates and compares strings

converted tabs to spaces

39ae1ab

alexrp reviewed Dec 24, 2012
View reviewed changes

Ingrater added 2 commits December 24, 2012 13:06

Fixed indentation for switch statement

bc292fd

Tabs as spaces again

5f23835

alexrp reviewed Dec 27, 2012
View reviewed changes

Ingrater added 3 commits February 18, 2013 19:07

Merge remote-tracking branch 'upstream/master' into pullTypeInfo

efd7edf

Conflicts: src/object.di

Made type info comparison improvement D-Style conform and fixed vario…

0f088b6

…us other minor issues.

Fixed .dup for the const(const(T)[]) case

b1dafea

MartinNowak reviewed Mar 23, 2013
View reviewed changes

Ingrater closed this May 8, 2013

Uh oh!

Remove allocation when comparing type info objects #370

Remove allocation when comparing type info objects #370

Uh oh!

Conversation

Ingrater commented Dec 24, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

9rnsr commented Dec 29, 2012

Uh oh!

Ingrater commented Dec 29, 2012

Uh oh!

alexrp commented Jan 21, 2013

Uh oh!

Ingrater commented Jan 21, 2013

Uh oh!

andralex commented Jan 22, 2013

Uh oh!

Ingrater commented Feb 20, 2013

Uh oh!

Ingrater commented Feb 26, 2013

Uh oh!

alexrp commented Feb 26, 2013

Uh oh!

WalterBright commented Mar 22, 2013

Uh oh!

Ingrater commented Mar 22, 2013

Uh oh!

MartinNowak commented Mar 23, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ingrater commented Mar 25, 2013

Uh oh!

MartinNowak commented Mar 25, 2013

Uh oh!

Ingrater commented Mar 25, 2013

Uh oh!

MartinNowak commented Mar 26, 2013

Uh oh!

MartinNowak commented Mar 26, 2013

Uh oh!

WalterBright commented Mar 26, 2013