Skip to content

profcheck: implement duplicate mappings check#108

Open
florianl wants to merge 5 commits intoopen-telemetry:mainfrom
florianl:profcheck-duplicate-mappings
Open

profcheck: implement duplicate mappings check#108
florianl wants to merge 5 commits intoopen-telemetry:mainfrom
florianl:profcheck-duplicate-mappings

Conversation

@florianl
Copy link
Copy Markdown
Member

No description provided.

Signed-off-by: Florian Lehner <dev@der-flo.net>
@florianl florianl requested a review from a team as a code owner April 18, 2026 10:08
Comment thread profcheck/check.go
if !(m.MemoryStart == 0 && m.MemoryLimit == 0) && !(m.MemoryStart < m.MemoryLimit) {
errs = errors.Join(errs, fmt.Errorf("[%d]: memory_start=%016x, memory_limit=%016x: must be both zero or start < limit", idx, m.MemoryStart, m.MemoryLimit))
}
if c.CheckDictionaryDuplicates {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If this style of duplicate entry check is fine for everyone, I can apply it to the other tables as well.

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Comment thread profcheck/check.go
Comment thread profcheck/check.go
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Comment thread profcheck/check.go Outdated
Comment on lines +245 to +246
slices.Sort(newMapping.attrIdxs)
slices.Sort(e.attrIdxs)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We're doing a lot of redundant operations here (like iterating uniqMappings for every mapping, sorting e.attrIdxs repeatedly instead of once) and depending on number of mappings, it could get expensive.

Can't we switch uniqMappings to be a map instead? We can keep uniqMapping as the key if we switch attrIdxs to be a string of sorted indices.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Can't we switch uniqMappings to be a map instead? We can keep uniqMapping as the key if we switch attrIdxs to be a string of sorted indices.

Unfortunatly it is not possible to use uniqMapping as key to a map, as uniqMapping contains a slice as a field. See https://go.dev/play/p/4-wpRA-KV3Q.

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants