Skip to content

[MC][NFC] Use std::map for AddressProbesMap #99553

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.

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

Merged
merged 4 commits into from
Jul 19, 2024

Conversation

aaupov
Copy link
Contributor

@aaupov aaupov commented Jul 18, 2024

AddressProbesMap is keyed by binary addresses, and it makes sense to
treat them as ordered. This also enables slicing by binary function/
binary basic block, to be used in BOLT
(#99554).

Test Plan: NFC

aaupov added 2 commits July 18, 2024 12:34
Created using spr 1.3.4
@aaupov aaupov requested a review from wlei-llvm July 18, 2024 19:57
Copy link
Contributor

@wlei-llvm wlei-llvm left a comment

Choose a reason for hiding this comment

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

LGTM.

aaupov added 2 commits July 18, 2024 20:59
Created using spr 1.3.4

[skip ci]
Created using spr 1.3.4
@aaupov aaupov changed the base branch from users/aaupov/spr/main.mcnfc-use-stdmap-for-addressprobesmap to main July 19, 2024 04:00
@aaupov aaupov merged commit 6c3aa62 into main Jul 19, 2024
5 of 8 checks passed
@aaupov aaupov deleted the users/aaupov/spr/mcnfc-use-stdmap-for-addressprobesmap branch July 19, 2024 04:00
@WenleiHe
Copy link
Member

AddressProbesMap is keyed by binary addresses, and it makes sense to treat them as ordered.

@aaupov this comment doesn't make sense. unordered_map is faster, and it was intentional. Order wasn't needed.

The intention of this patch is only obvious with the other patch that uses the map, so it'd be better to merge the two in one patch.

@aaupov
Copy link
Contributor Author

aaupov commented Jul 19, 2024

AddressProbesMap is keyed by binary addresses, and it makes sense to treat them as ordered.

@aaupov this comment doesn't make sense. unordered_map is faster, and it was intentional. Order wasn't needed.

Sorry, the wording wasn't ideal. I was trying to convey that addresses can be treated as ordered keys (unlike e.g. GUIDs), which allows certain lookups as in the follow-up diff.

The intention of this patch is only obvious with the other patch that uses the map, so it'd be better to merge the two in one patch.

I see your point, but I tried to explain the intention in the message. I applied the usual logic of keeping changes small and isolated, but perhaps in this case the changes were best left coupled.

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.

3 participants