Remove AA faketypeinfo requirement #20853
Conversation
|
Thanks for your pull request, @schveiguy! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.
|
schveiguy
force-pushed
the
fixaafaketi
branch
2 times, most recently
from
4b54cda to
716a65a
Compare
| 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 |
There was a problem hiding this comment.
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.
…ht be collected before the element is collected.
schveiguy
force-pushed
the
fixaafaketi
branch
from
716a65a to
f905983
Compare
| } | ||
|
|
||
| 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; |
There was a problem hiding this comment.
Note: optimization. No need to test for a static array when we know it's a struct.
| // unfortunately, need to recalculate this every time. | ||
| auto valoff = talign(ti.key.tsize, ti.value.talign); | ||
| ti.value.destroy(p + valoff); | ||
| } |
There was a problem hiding this comment.
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.
|
Not a fan of creating the entry typeinfo for each AA, but that info has been pretty important for precise scanning IIRC.
I never bought that argument: the dmd-based language server running as a 64-bit background process for Visual D eats tons of memory without the precise GC (even with precise scanning it still does eventually, I recently killed it at 56 GB). I never switched to LDC for building the server because LDC does not support precise scanning of the data segment. Without AAs being templated I think the better solution would be to let the compiler generate the TypeInfo for the (key,value)-pair and put it into TypeInfo_AssociativeArray. |
So my other thought on this, but I didn't bring it up, is to also invoke RTInfo for |
This points at a memory leak rather than a precise scanning problem. I don't want to dismiss it out of hand, because I don't know the true cause, but I have found problems that have been around for decades recently when focusing on this. Mostly due to having the new GC not fixing what was thought to be a problem with the old GC. As an example, see here: dlang/phobos#9084 I'm trying to fix a case at a work project where I get a segfault very rarely, and this is the cause. Help me try and figure out how we can solve this problem without allocating memory, and without crippling the precise scanner. I had considered still passing in the AA typeinfo, and having the precise scanner recognize it should dig into the key/value typeinfos to fetch the bits (or defer to the runtime for populating the info). But this doesn't help for large entries that would store just the pointer to the bit info. A possibility is to store multiple pointers in there? How many pointers do we get, one per page, right? Can we add a second pointer and a pad size? I don't know the precise scanner layout of things. All this is a lot more complex than just defaulting to conservative scanning, so it would take time to solve. |
Oh, I see, it's C-mallocing that. I can work with that, just malloc extra space! So there is a possibility here that we can keep precise scanning, let me think about this for a while, I think I don't need to change much, just hook AA typeinfos so we either fill in the bits (small allocations) or store both pointers + offsets (large allocations) |
@rainers If you know of a good way to make this happen, it would actually be a lot cleaner. I am completely lost when it comes to compiler development. |
|
I think adding a proper type info for the Entry struct is the cleaner way. It should contain the RTInfo as well as any dtor pointers. I can have a look if it's easy to add, but I'll probably only get to that in a couple of days. |
OK, I'll hold off on my other plan (as outlined above, with some adjustments to how large pointer bitmaps are stored) until I hear back from you. If you decide this isn't something you have time for, we can at least go that route. I took a look myself, and the way the RTInfo is generated is extremely tied to classes and structs (it's in aggregate.d). So just extending this to a builtin type such as AA isn't an easy lift. |
If we can simply say, whenever we decide we have a new AA, we instantiate the template Man, I wish I was more knowledgeable at compilers... |
| @@ -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)) | |||
There was a problem hiding this comment.
Need to undo this. I changed the name because I was having segfaults and gdb was confused about what p was.
|
Closing in favor of much more straightforward #20863 |
Fixes #17503.
One "regression"-ish from this: AA elements will no longer be precisely scanned by the precise GC. Explanation: The fake typeinfo used an additional allocation to create a bitmap with pointer data. This too could be collected, and is not worth maintaining.
If in the future we move to an all-template hook system with AAs, the precise scanning data (and even the element typeinfo) will be trivial to use (see how newaa does it). Until then, I think it's worth this mini-regression, since precise scanning is not that useful on modern 64-bit systems and is therefore seldom enabled.