pkg/proc: support swiss table map implementation #3838
Conversation
aarzilli
changed the title
|
Marking WIP until the swisstable implementation in go doesn't have TODO comments. |
aarzilli
changed the title
|
The swissmap backend has been enabled by default on 1.24 so I think we can review this now. |
| @@ -1992,12 +1991,7 @@ func (v *Variable) loadMap(recurseLevel int, cfg LoadConfig) { | |||
| errcount := 0 | |||
| for it.next() { | |||
| key := it.key() | |||
There was a problem hiding this comment.
How were indirect keys handled previously? It looks like they simply weren't supported at all? (Unlike indirect values, which are handled just below)
(I'm just curious, this does seem to be handled for old and swiss maps in the new implementation.)
There was a problem hiding this comment.
We didn't handle either. I missed it when I first implemented it years ago and nobody ever reported it as a bug.
I've added support to both with this PR, for classic map is the code in (*mapIteratorClassic).key() that does the k.Kind == reflect.Ptr && !it.keyTypeIsPtr, for swiss maps it's in next, the if for it.curKey.Kind == reflect.Ptr && !it.keyTypeIsPtr (and their equivalents for value).
| return false | ||
| } | ||
|
|
||
| if it.curKey.Kind == reflect.Ptr && !it.keyTypeIsPtr { |
There was a problem hiding this comment.
This is how you detect indirect keys/values.
IIUC, the idea here is that if the key field has a pointer type, but the key type itself is not a pointer, then it must be an indirect key. On the other hand, if the key type is a pointer, then it is small enough to never be indirect.
I don't see any problems with this logic, though IMO it is worth a comment.
FWIW, the way the runtime does this is that the map type flag field records whether the key/value are indirect. It seems you could do that too, though I'm not sure it would strictly be better or worse.
There was a problem hiding this comment.
The map type is no longer stored in the map object, if I'm not mistaken to get to the map type I would have to use the DW_AT_go_runtime_type. It's very annoying.
pkg/proc/mapiter.go
Outdated
|
|
||
| // loadTypes determines the correct type for it.dirPtr: the linker records | ||
| // this type as **table but in reality it is either *[dirLen]*table for | ||
| // large maps or *group for small maps, when it.dirLen <= 0. |
There was a problem hiding this comment.
nit: in the submitted version, dirLen is always 0 for small maps.
pkg/proc/mapiter.go
Outdated
| } | ||
| } | ||
|
|
||
| for ; it.groupIdx <= uint64(it.tab.groups.Len); it.nextGroup() { |
There was a problem hiding this comment.
it.tab.groups.Len is the array length, right? Thus shouldn't this be < not <=?
pkg/proc/mapiter.go
Outdated
| return it.curValue | ||
| } | ||
|
|
||
| func (it *mapIteratorSwiss) slotIsEmpty(k uint32) bool { |
There was a problem hiding this comment.
nit: slotIsEmptyOrDeleted
(The implementation is correct; this matches empty or deleted slots)
pkg/proc/mapiter.go
Outdated
| } | ||
|
|
||
| it.slotIdx++ | ||
| if it.slotIdx >= uint32(it.group.slots.Len) { |
There was a problem hiding this comment.
I don't think this is strictly necessary. If you simply return after it.slotIdx++, then the next call will fail the for ; it.slotIdx < uint32(it.group.slots.Len) condition and fall through to the end of the group loop at L514.
| it.dirPtr.RealType = it.dirPtr.DwarfType | ||
| it.dirPtr = it.dirPtr.maybeDereference() | ||
| it.dirLen = 1 | ||
| it.tab = &swissTable{groups: it.dirPtr} // so that we don't try to load this later on |
There was a problem hiding this comment.
I wanted to do this in the runtime implementation but couldn't afford an allocation. You are lucky to get that luxury!
There was a problem hiding this comment.
Cross-process memory reads are slow anyway.
Adds support for the swiss table implementation for Go's maps.
Adds support for the swiss table implementation for Go's maps.