Skip to content

Remove AA faketypeinfo requirement #20853

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
schveiguy wants to merge 2 commits into from
Closed

Remove AA faketypeinfo requirement #20853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f905983
Fix #17503 - AAs should not GC allocate a fake typeinfo that might be
schveiguy Feb 11, 2025
602393c
Add test case for fake typeinfo clearing.
schveiguy Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion druntime/src/core/internal/gc/impl/conservative/gc.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5419,7 +5419,7 @@ void undefinedWrite(T)(ref T var, T value) nothrow
private void adjustAttrs(ref uint attrs, const TypeInfo ti) nothrow
{
bool hasContext = ti !is null;
if((attrs & BlkAttr.FINALIZE) && hasContext && typeid(ti) is typeid(TypeInfo_Struct))
if((attrs & BlkAttr.FINALIZE) && hasContext && (typeid(ti) !is typeid(TypeInfo_Class)))
attrs |= BlkAttr.STRUCTFINAL;
else
// STRUCTFINAL now just means "has a context pointer added to the block"
Expand Down
5 changes: 4 additions & 1 deletion druntime/src/core/internal/newaa.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ struct Impl
Bucket[] buckets;
uint used;
uint deleted;
TypeInfo_Struct entryTI;
const TypeInfo entryTI;
uint firstUsed;
immutable uint keysz;
immutable uint valsz;
Expand All @@ -57,6 +57,7 @@ struct Impl
none = 0x0,
keyHasPostblit = 0x1,
hasPointers = 0x2,
hasDtors = 0x04,
}
}

Expand Down Expand Up @@ -147,6 +148,8 @@ AAShell makeAA(K, V)(V[K] src) @trusted
flags |= flags.keyHasPostblit;
static if (hasIndirections!E)
flags |= flags.hasPointers;
static if (hasElaborateDestructor!E)
flags |= flags.hasDtors;
return flags;
} ();
// return the new implementation
Expand Down
208 changes: 41 additions & 167 deletions druntime/src/rt/aaA.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,18 @@ private:
flags |= Flags.keyHasPostblit;
if ((ti.key.flags | ti.value.flags) & 1)
flags |= Flags.hasPointers;

entryTI = fakeEntryTI(this, ti.key, ti.value);
if (hasDtor(unqualify(ti.key)) || hasDtor(unqualify(ti.value)))
{
// need the type info for the destructors.
entryTI = ti;
flags |= Flags.hasDtors;
}
}

Bucket[] buckets;
uint used;
uint deleted;
TypeInfo_Struct entryTI;
const TypeInfo entryTI;
uint firstUsed;
immutable uint keysz;
immutable uint valsz;
Expand All @@ -88,6 +92,7 @@ private:
none = 0x0,
keyHasPostblit = 0x1,
hasPointers = 0x2,
hasDtors = 0x4,
}

@property size_t length() const pure nothrow @nogc @safe
Expand Down Expand Up @@ -210,182 +215,58 @@ Bucket[] allocBuckets(size_t dim) @trusted pure nothrow

private void* allocEntry(scope const Impl* aa, scope const void* pkey)
{
import rt.lifetime : _d_newitemU;
import core.stdc.string : memcpy, memset;

immutable akeysz = aa.valoff;
void* res = void;
if (aa.entryTI)
res = _d_newitemU(aa.entryTI);
else
immutable akeysz = aa.keysz;
immutable allocsz = aa.valoff + aa.valsz;
auto flags = (aa.flags & Impl.Flags.hasPointers) ? 0 : GC.BlkAttr.NO_SCAN;
if (aa.flags & Impl.Flags.hasDtors)
{
auto flags = (aa.flags & Impl.Flags.hasPointers) ? 0 : GC.BlkAttr.NO_SCAN;
res = GC.malloc(akeysz + aa.valsz, flags);
assert(aa.entryTI !is null);
flags |= GC.BlkAttr.FINALIZE;
}
auto res = GC.malloc(allocsz, flags, aa.entryTI);

memcpy(res, pkey, aa.keysz); // copy key
memset(res + akeysz, 0, aa.valsz); // zero value
memcpy(res, pkey, akeysz); // copy key
memset(res + akeysz, 0, allocsz - akeysz); // zero everything after
Copy link
Member Author

@schveiguy schveiguy Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Note, I changed this to memset also the gap between key and value, because I've seen what problems not doing that can cause, and it's probably at most 15 extra bytes to memset with the same call.


return res;
}

package void entryDtor(void* p, const TypeInfo_Struct sti)
package void finalizeEntry(void* p, TypeInfo_AssociativeArray ti)
{
// key and value type info stored after the TypeInfo_Struct by tiEntry()
auto sizeti = __traits(classInstanceSize, TypeInfo_Struct);
auto extra = cast(const(TypeInfo)*)(cast(void*) sti + sizeti);
extra[0].destroy(p);
extra[1].destroy(p + talign(extra[0].tsize, extra[1].talign));
import rt.lifetime : unqualify;

ti.key.destroy(p);
if (hasDtor(unqualify(ti.value)))
{
// unfortunately, need to recalculate this every time.
auto valoff = talign(ti.key.tsize, ti.value.talign);
ti.value.destroy(p + valoff);
}
Copy link
Member Author

@schveiguy schveiguy Feb 11, 2025

Choose a reason for hiding this comment

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

This recalculation on every finalize is unfortunate. If there were a way to store this offset in the TypeInfo_AssociativeArray, that would be ideal. It doesn't even need to be thread-aware, because it's a constant.

But maybe it's not a huge problem, because eventually we will be doing this via templates and just use the Entry structure directly.

}

private bool hasDtor(const TypeInfo ti) pure nothrow
{
import rt.lifetime : unqualify;

if (typeid(ti) is typeid(TypeInfo_Struct))
if ((cast(TypeInfo_Struct) cast(void*) ti).xdtor)
return true;
return (cast(TypeInfo_Struct) cast(void*) ti).xdtor !is null;
Copy link
Member Author

@schveiguy schveiguy Feb 11, 2025

Choose a reason for hiding this comment

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

Note: optimization. No need to test for a static array when we know it's a struct.

if (typeid(ti) is typeid(TypeInfo_StaticArray))
return hasDtor(unqualify(ti.next));

return false;
}

private immutable(void)* getRTInfo(const TypeInfo ti) pure nothrow
{
// classes are references
const isNoClass = ti && typeid(ti) !is typeid(TypeInfo_Class);
return isNoClass ? ti.rtInfo() : rtinfoHasPointers;
}

// build type info for Entry with additional key and value fields
TypeInfo_Struct fakeEntryTI(ref Impl aa, const TypeInfo keyti, const TypeInfo valti) nothrow
unittest
{
import rt.lifetime : unqualify;

auto kti = unqualify(keyti);
auto vti = unqualify(valti);

// figure out whether RTInfo has to be generated (indicated by rtisize > 0)
enum pointersPerWord = 8 * (void*).sizeof * (void*).sizeof;
auto rtinfo = rtinfoNoPointers;
size_t rtisize = 0;
immutable(size_t)* keyinfo = void;
immutable(size_t)* valinfo = void;
if (aa.flags & Impl.Flags.hasPointers)
static immutable(void)* getRTInfo(const TypeInfo ti) pure nothrow
{
// classes are references
keyinfo = cast(immutable(size_t)*) getRTInfo(keyti);
valinfo = cast(immutable(size_t)*) getRTInfo(valti);

if (keyinfo is rtinfoHasPointers && valinfo is rtinfoHasPointers)
rtinfo = rtinfoHasPointers;
else
rtisize = 1 + (aa.valoff + aa.valsz + pointersPerWord - 1) / pointersPerWord;
}
bool entryHasDtor = hasDtor(kti) || hasDtor(vti);
if (rtisize == 0 && !entryHasDtor)
return null;

// save kti and vti after type info for struct
enum sizeti = __traits(classInstanceSize, TypeInfo_Struct);
void* p = GC.malloc(sizeti + (2 + rtisize) * (void*).sizeof);
import core.stdc.string : memcpy;

memcpy(p, __traits(initSymbol, TypeInfo_Struct).ptr, sizeti);

auto ti = cast(TypeInfo_Struct) p;
auto extra = cast(TypeInfo*)(p + sizeti);
extra[0] = cast() kti;
extra[1] = cast() vti;

static immutable tiMangledName = "S2rt3aaA__T5EntryZ";
ti.mangledName = tiMangledName;

ti.m_RTInfo = rtisize > 0 ? rtinfoEntry(aa, keyinfo, valinfo, cast(size_t*)(extra + 2), rtisize) : rtinfo;
ti.m_flags = ti.m_RTInfo is rtinfoNoPointers ? cast(TypeInfo_Struct.StructFlags)0 : TypeInfo_Struct.StructFlags.hasPointers;

// we don't expect the Entry objects to be used outside of this module, so we have control
// over the non-usage of the callback methods and other entries and can keep these null
// xtoHash, xopEquals, xopCmp, xtoString and xpostblit
immutable entrySize = aa.valoff + aa.valsz;
ti.m_init = (cast(ubyte*) null)[0 .. entrySize]; // init length, but not ptr

if (entryHasDtor)
{
// xdtor needs to be built from the dtors of key and value for the GC
ti.xdtorti = &entryDtor;
ti.m_flags |= TypeInfo_Struct.StructFlags.isDynamicType;
}

ti.m_align = cast(uint) max(kti.talign, vti.talign);

return ti;
}

// build appropriate RTInfo at runtime
immutable(void)* rtinfoEntry(ref Impl aa, immutable(size_t)* keyinfo,
immutable(size_t)* valinfo, size_t* rtinfoData, size_t rtinfoSize) pure nothrow
{
enum bitsPerWord = 8 * size_t.sizeof;

rtinfoData[0] = aa.valoff + aa.valsz;
rtinfoData[1..rtinfoSize] = 0;

void copyKeyInfo(string src)()
{
size_t pos = 1;
size_t keybits = aa.keysz / (void*).sizeof;
while (keybits >= bitsPerWord)
{
rtinfoData[pos] = mixin(src);
keybits -= bitsPerWord;
pos++;
}
if (keybits > 0)
rtinfoData[pos] = mixin(src) & ((size_t(1) << keybits) - 1);
const isNoClass = ti && typeid(ti) !is typeid(TypeInfo_Class);
return isNoClass ? ti.rtInfo() : rtinfoHasPointers;
}

if (keyinfo is rtinfoHasPointers)
copyKeyInfo!"~size_t(0)"();
else if (keyinfo !is rtinfoNoPointers)
copyKeyInfo!"keyinfo[pos]"();

void copyValInfo(string src)()
{
size_t bitpos = aa.valoff / (void*).sizeof;
size_t pos = 1;
size_t dstpos = 1 + bitpos / bitsPerWord;
size_t begoff = bitpos % bitsPerWord;
size_t valbits = aa.valsz / (void*).sizeof;
size_t endoff = (bitpos + valbits) % bitsPerWord;
for (;;)
{
const bits = bitsPerWord - begoff;
size_t s = mixin(src);
rtinfoData[dstpos] |= s << begoff;
if (begoff > 0 && valbits > bits)
rtinfoData[dstpos+1] |= s >> bits;
if (valbits < bitsPerWord)
break;
valbits -= bitsPerWord;
dstpos++;
pos++;
}
if (endoff > 0)
rtinfoData[dstpos] &= (size_t(1) << endoff) - 1;
}

if (valinfo is rtinfoHasPointers)
copyValInfo!"~size_t(0)"();
else if (valinfo !is rtinfoNoPointers)
copyValInfo!"valinfo[pos]"();

return cast(immutable(void)*) rtinfoData;
}

unittest
{
void test(K, V)()
{
static struct Entry
Expand All @@ -404,21 +285,11 @@ unittest
assert(!(impl.flags & Impl.Flags.hasPointers));
assert(impl.entryTI is null);
}
else if (valrti is rtinfoHasPointers && keyrti is rtinfoHasPointers)
else
{
assert(impl.flags & Impl.Flags.hasPointers);
assert(impl.entryTI is null);
}
else
{
auto rtInfo = cast(size_t*) impl.entryTI.rtInfo();
auto refInfo = cast(size_t*) typeid(Entry).rtInfo();
assert(rtInfo[0] == refInfo[0]); // size
enum bytesPerWord = 8 * size_t.sizeof * (void*).sizeof;
size_t words = (rtInfo[0] + bytesPerWord - 1) / bytesPerWord;
foreach (i; 0 .. words)
assert(rtInfo[1 + i] == refInfo[i + 1]);
}
}
test!(long, int)();
test!(string, string);
Expand Down Expand Up @@ -559,10 +430,10 @@ extern (C) void* _aaGetX(scope AA* paa, const TypeInfo_AssociativeArray ti,
immutable hash = calcHash(pkey, aa);

// found a value => return it
if (auto p = aa.findSlotLookup(hash, pkey, ti.key))
if (auto q = aa.findSlotLookup(hash, pkey, ti.key))
Copy link
Member Author

@schveiguy schveiguy Feb 12, 2025

Choose a reason for hiding this comment

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

Need to undo this. I changed the name because I was having segfaults and gdb was confused about what p was.

{
found = true;
return p.entry + aa.valoff;
return q.entry + aa.valoff;
}

auto p = aa.findSlotInsert(hash);
Expand Down Expand Up @@ -795,7 +666,7 @@ extern (C) Impl* _d_assocarrayliteralTX(const TypeInfo_AssociativeArray ti, void
aa.firstUsed = min(aa.firstUsed, cast(uint)(p - aa.buckets.ptr));
actualLength++;
}
else if (aa.entryTI && hasDtor(ti.value))
else if ((aa.flags & Impl.Flags.hasDtors) && hasDtor(ti.value))
{
// destroy existing value before overwriting it
ti.value.destroy(p.entry + off);
Expand Down Expand Up @@ -977,8 +848,11 @@ unittest
aa1 = null;
aa2 = null;
aa3 = null;
GC.runFinalizers((cast(char*)&entryDtor)[0 .. 1]);
assert(T.dtor == 6 && T.postblit == 2);
auto entryDtor = typeid(T).xdtor;
assert(entryDtor !is null);
GC.runFinalizers((cast(char*)entryDtor)[0 .. 1]);
assert(T.postblit == 2);
assert(T.dtor == 6);
}

// Ensure the newaa struct layout (used for static initialization) is in sync
Expand Down
Loading
Loading