[WIP] Lower newExp #2508

edi33416 · 2019-03-08T23:57:32Z

Add a templated version of _d_newitemT that drops TypeInfo and uses the type known at compile time.

This is required by this dmd pr

dlang-bot · 2019-03-08T23:57:33Z

Thanks for your pull request and interest in making D better, @edi33416! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2508"

thewilsonator · 2019-03-09T06:00:51Z

src/object.d

+
+    static if (is(T == struct) && is(typeof({ T t; t.__xdtor; })))
+        return size_t.sizeof;
+    return 0;


This will complain about unreachable statement if the above static if is true.

thewilsonator · 2019-03-09T06:08:17Z

src/object.d

+    import core.internal.traits : Unqual;
+
+    auto ti = typeid(Unqual!T);
+    auto flags = !(ti.flags & 1) ? GC.BlkAttr.NO_SCAN : 0;


I thought the whole point was to avoid typeid? Surely you should be able to use a compiler trait for ti.flags & 1.

Otherwise one can be added.

I haven't found a compiler trait. Maybe I missed it. Anyone know about one?

No, not that I can see. But add one. I think it's currently handled here: https://github.com/dlang/dmd/blob/bf26fb4ffc173f016eae9c241bcfd5a88da92957/src/dmd/toobj.d#L1291-L1327.

I didn't know what (ti.flags & 1) meant, so I had to go spelunking in DMD source code to see what it means. Assuming I'm reading the DMD source code correctly, it's isCOMclass. At a minimum maybe you could do something like enum COMClass = 1 to save the next poor soul the trouble.

Second, according to https://dlang.org/spec/interface.html#com-interfaces does that flag mean that it inherits from core.stdc.windows.com.IUnknown? Is there an existing trait that can check for that? Or can you use __traits(compiles,...) to achieve the desired result?

It's really bad:

druntime/src/object.d

Lines 1313 to 1315 in 4ad638f

/** Get flags for type: 1 means GC should scan for pointers,

2 means arg of this type is passed in XMM register */

@property uint flags() nothrow pure const @safe @nogc { return 0; }

Especially as there is this easily confusing beauty (used for TypeInfo_Class.m_flags) you referred to:

druntime/src/object.d

Lines 1969 to 1980 in 4ad638f

enum ClassFlags : uint

{

isCOMclass = 0x1,

noPointers = 0x2,

hasOffTi = 0x4,

hasCtor = 0x8,

hasGetMembers = 0x10,

hasTypeInfo = 0x20,

isAbstract = 0x40,

isCPPclass = 0x80,

hasDtor = 0x100,

}

druntime/src/object.d

Line 2194 in 4ad638f

override @property uint flags() nothrow pure const { return m_flags; }

Now I'm thoroughly confused.

This one is for structs, not classes. TypeInfo_Class.flags() always returns 1 as TypeInfo_Class tries to describe both the reference and the instance of a class at the same time (which is causing trouble elsewhere, too, e.g. when trying to specify the type of memory for the precise GC).

Is there an existing trait that can check for that? Or can you use __traits(compiles,...) to achieve the desired result?

I guess is(T : IUnknown) can be used.

jacob-carlborg · 2019-03-09T11:11:03Z

src/object.d

+{
+    import core.stdc.string;
+    auto p = ___d_newitemU!T();
+    auto init = T.init;


Maybe static const even?

jacob-carlborg · 2019-03-09T11:13:29Z

src/object.d

+{
+    import core.stdc.string;
+    auto p = ___d_newitemU!T();
+    memset(p, 0, T.sizeof);


The comment says "for item with non-zero initializer". But doesn't this initialize to zero?

jacob-carlborg · 2019-03-09T11:14:35Z

src/object.d

+{
+    import core.stdc.string;
+    auto p = ___d_newitemU!T();
+    memset(p, 0, T.sizeof);


Is there a problem of using T.init when it's zero? Why can't the function below be used for all types?

Maybe memset is faster than memcpy? Not sure; I'll do some benchmarking

Zero initialisers didn't actually get emitted as such; the pointer then is zero. This only applies to TypeInfo, of course.

edi33416 · 2019-03-11T16:20:16Z

src/object.d

+// have a destructor
+private size_t structTypeInfoSize(T)() pure nothrow @nogc
+{
+    // can't use because it's in rt.lifetime


What about this? Any solutions?

The simplest would be to make callStructDtorsDuringGC package and import rt.lifetime.

Can't import rt.lifetime. You can't import anything from rt/* since it's not in mak/COPY's $(IMPDIR).

Maybe I'm missing something, as the GC is able to import from rt/util/

Then you can do a hack, declare an extern variable and use pragma(mangle) to get the same mangling as for callStructDtorsDuringGC declared in rt.lifetime.

We can do even better. Here's an example from the existing code:

import core.internal.traits : externDFunc; alias rt_tlsgc_init = externDFunc!("rt.tlsgc.init", void* function() nothrow @nogc);

~~Does that work with non-function types?~~ That won't work for non-function types. The template explicitly checks for a function type.

You could add an extern (C) function in lifetime.d to return callStructDtorsDuringGC and then just declare that prototype in object.d. You can already find precedent for that technique in object.d (e.g.

druntime/src/object.d

Lines 3909 to 3910 in 4ad638f

extern (C) void _d_arrayshrinkfit(const TypeInfo ti, void[] arr) nothrow;

extern (C) size_t _d_arraysetcapacity(const TypeInfo ti, size_t newcapacity, void[]* arrptr) pure nothrow;

)

Though, this isn't much different than @jacob-carlborg's suggestion.

edi33416 · 2019-03-11T16:46:05Z

src/object.d

+    //if (!callStructDtorsDuringGC)
+        //return 0;
+
+    static if (is(T == struct) && is(typeof({ T t; t.__xdtor; })))


Is this the same as TypeInfo_Struct.xdtor()?

I'm afraid I don't know at this time. I'm sorry.

andralex · 2019-03-11T16:55:17Z

src/object.d

+    }
+    else
+    {
+        auto init = T.init;


static __gshared immutable init = T.init;

Isn't __gshared redundant when it's immutable?

jacob-carlborg · 2019-03-11T17:13:37Z

src/object.d

+    return cast(T*) p;
+}
+
+// Same as above, for item with non-zero initializer.


This is a public symbol, should it has a Ddoc comment?

jacob-carlborg · 2019-03-11T17:16:44Z

src/object.d

+ * Allocate an uninitialized non-array item.
+ * This is an optimization to avoid things needed for arrays like the __arrayPad(size).
+ */
+private T* ___d_newitemU(T)()


I don't think these names, ___d_newitemU and ___d_newitemT, are very descriptive. It's difficult to know what's the difference just by looking at their names. Perhaps ___d_newitemU instead should contain the word "allocate".

jacob-carlborg · 2019-03-11T17:17:05Z

src/object.d

+}
+
+/*
+ * Allocate an uninitialized non-array item.


What's an item? It's not very descriptive.

jacob-carlborg · 2019-03-11T17:25:07Z

src/object.d

+    import core.internal.traits : Unqual;
+
+    auto ti = typeid(Unqual!T);
+    auto flags = !(ti.flags & 1) ? GC.BlkAttr.NO_SCAN : 0;


Avoid using magic constants like this 1.

kinke · 2019-03-17T16:15:32Z

Add a templated version of _d_newitemT that drops TypeInfo

It clearly doesn't (just moving from param to function body), so what's the benefit of this? Right now it's just bloating code and slightly increasing compile times if I understand correctly, and replaces 1-2 runtime-ifs with static ifs.

kinke · 2019-03-17T18:09:03Z

To reduce bloat and still allow for future revamping without TypeInfo or whatever, and including some potential performance improvements (wrt. status quo in master) due to static size for initialization, we could also have something like

T* _d_newValueOnHeap(T, bool initialize)()
{
    import core.internal.traits : Unqual;
    return cast(T*) newValueOnHeap!(Unqual!T, initialize)();
}

private T* newValueOnHeap(T, bool initialize)() @trusted
{
    enum hasDtor = is(T == struct) && __traits(hasMember, T, "__xdtor");
    auto ti = typeid(T);
    auto p = cast(T*) allocateValueOnHeap!(hasDtor)(T.sizeof, ti);

    static if (initialize)
    {
        import core.stdc.string;

        static if (__traits(isZeroInit, T))
            memset(p, 0, T.sizeof);
        else
            memcpy(p, ti.initializer().ptr, T.sizeof);
    }

    return p;
}

private void* allocateValueOnHeap(bool hasDtor)(size_t size, TypeInfo ti)
{
    import core.memory;

    const fullSize = size + (hasDtor ? size_t.sizeof : 0);
    auto flags = !(ti.flags & 1) ? GC.BlkAttr.NO_SCAN : 0;
    static if (hasDtor)
        flags |= GC.BlkAttr.STRUCTFINAL | GC.BlkAttr.FINALIZE;
    auto blkInf = GC.qalloc(fullSize, flags, ti);
    auto p = blkInf.base;

    static if (hasDtor)
        *cast(TypeInfo*)(p + blkInf.size - size_t.sizeof) = cast() ti;

    return p;
}

JinShil · 2019-04-07T00:01:40Z

I'm ok with a direct, imperfect translation. Once DMD is modified to call the template correctly, the implementation of the template can be improved with followup PRs and additional unittests; we just need to ensure the API is correct for now, IMO.

kinke · 2019-04-07T00:18:19Z

I was disappointed by the lack of rationale and the reviews just tackling the surface, boring stylistic things. And then no response for 20 days. I'd be okay with following up myself (although the 2nd template param would require a DMD change too, so doing it properly the first time would be less hassle).

JinShil · 2019-04-07T00:20:05Z

To aid this review, I think this PR could use a couple of unittests that call the __newItem template directly, just to test the implementation. At least I found that useful when working on #2264.

n8sh · 2019-04-12T22:26:08Z

src/object.d

+    auto p = (() @trusted => ___d_newitemU!T())();
+    static if (__traits(isZeroInit, T))
+    {
+        () @trusted { memset(cast(Unqual!T *) p, 0, T.sizeof); }();


cast(void*), no need for Unqual!T*.

n8sh · 2019-04-13T00:15:53Z

Once DMD is modified to call the template correctly, the implementation of the template can be improved

I agree with kinke's comment that this PR as it stands is a pessimization. Making it not worse than what it is replacing should be done before it enters use, IMO.

Lower newExp

33b1ea3

edi33416 requested review from andralex and MartinNowak as code owners March 8, 2019 23:57

edi33416 changed the title ~~Lower newExp~~ [WIP] Lower newExp Mar 8, 2019

dlang-bot added the WIP Work In Progress - not ready for review or pulling label Mar 8, 2019

thewilsonator reviewed Mar 9, 2019

View reviewed changes

jacob-carlborg reviewed Mar 9, 2019

View reviewed changes

Use T to check if isZeroInitialized

4cabd7c

edi33416 commented Mar 11, 2019

View reviewed changes

andralex reviewed Mar 11, 2019

View reviewed changes

jacob-carlborg reviewed Mar 11, 2019

View reviewed changes

src/object.d

}

/*

* Allocate an uninitialized non-array item.

Copy link

Contributor

jacob-carlborg Mar 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's an item? It's not very descriptive.

jacob-carlborg reviewed Mar 11, 2019

View reviewed changes

n8sh reviewed Apr 12, 2019

View reviewed changes

dlang-bot added Needs Rebase needs a `git rebase` performed Needs Work stalled labels May 27, 2021

RazvanN7 closed this Dec 13, 2021

	/** Get flags for type: 1 means GC should scan for pointers,
	2 means arg of this type is passed in XMM register */
	@property uint flags() nothrow pure const @safe @nogc { return 0; }

	enum ClassFlags : uint
	{
	isCOMclass = 0x1,
	noPointers = 0x2,
	hasOffTi = 0x4,
	hasCtor = 0x8,
	hasGetMembers = 0x10,
	hasTypeInfo = 0x20,
	isAbstract = 0x40,
	isCPPclass = 0x80,
	hasDtor = 0x100,
	}

	extern (C) void _d_arrayshrinkfit(const TypeInfo ti, void[] arr) nothrow;
	extern (C) size_t _d_arraysetcapacity(const TypeInfo ti, size_t newcapacity, void[]* arrptr) pure nothrow;

[WIP] Lower newExp #2508

[WIP] Lower newExp #2508

Conversation

edi33416 commented Mar 8, 2019

dlang-bot commented Mar 8, 2019

Bugzilla references

Testing this PR locally

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinShil Apr 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnadlinger Mar 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacob-carlborg Mar 12, 2019 • edited Loading

Choose a reason for hiding this comment

JinShil Apr 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andralex Mar 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinke commented Mar 17, 2019 • edited Loading

kinke commented Mar 17, 2019 • edited Loading

JinShil commented Apr 7, 2019 • edited Loading

kinke commented Apr 7, 2019 • edited Loading

JinShil commented Apr 7, 2019 • edited Loading

Choose a reason for hiding this comment

n8sh commented Apr 13, 2019

JinShil Apr 6, 2019 •

edited

Loading

dnadlinger Mar 11, 2019 •

edited

Loading

jacob-carlborg Mar 12, 2019 •

edited

Loading

JinShil Apr 7, 2019 •

edited

Loading

andralex Mar 11, 2019 •

edited

Loading

kinke commented Mar 17, 2019 •

edited

Loading

kinke commented Mar 17, 2019 •

edited

Loading

JinShil commented Apr 7, 2019 •

edited

Loading

kinke commented Apr 7, 2019 •

edited

Loading

JinShil commented Apr 7, 2019 •

edited

Loading