upstream: Uses a more efficient type for ClusterMap

[birenroy]

This change replaces std::map with absl::btree_map, which is more efficient in terms of memory allocations and CPU/memory operations per lookup.

Commit Message: upstream: Uses a more efficient type for ClusterMap
Additional Description:
Risk Level:
Testing: ran unit and integration tests locally
Docs Changes:
Release Notes:
Platform Specific Features:

[birenroy]

/assign @adisuissa

[birenroy]

/assign @paul-r-gall

[birenroy]

I'm having a hard time understanding why //test/extensions/config_subscription/grpc:xds_failover_integration_test is failing. If you have any ideas, they would be much appreciated.

[adisuissa]

Oh, I actually have a different PR (#44015) in draft mode at the moment (to make sure it works with all the tests), that takes a different approach by removing the need to use clusters() in many places.
What do you think are the performance tradeoffs between the two approaches (keeping the ordered map vs. avoiding the clusters() invocation)?

[paul-r-gall]

@adisuissa I think your PR resolves an orthogonal problem.

[adisuissa]

@adisuissa I think your PR resolves an orthogonal problem.

If the ordering is still needed, then the other PR needs to support temporary sorting prior to iterating, which makes the improvement (avoiding the creation of temporary maps/lists) moot.

[birenroy]

Alright, I've moved back to using absl::btree_map, which maintains the ordering property. Please take another look.

[birenroy]

/retest

[pianiststickman]

@adisuissa I think your PR resolves an orthogonal problem.

If the ordering is still needed, then the other PR needs to support temporary sorting prior to iterating, which makes the improvement (avoiding the creation of temporary maps/lists) moot.

I don't quite understand this comment - any caller of clusters() can't be relying on its order, because the structure returned by clusters() uses the unordered absl::flat_hash_map already.

[birenroy]

/assign @RyanTheOptimist

[adisuissa]

@adisuissa I think your PR resolves an orthogonal problem.

If the ordering is still needed, then the other PR needs to support temporary sorting prior to iterating, which makes the improvement (avoiding the creation of temporary maps/lists) moot.

I don't quite understand this comment - any caller of clusters() can't be relying on its order, because the structure returned by clusters() uses the unordered absl::flat_hash_map already.

That's an important observation. I think what threw me off is the comment, that was added a while ago (implying that this change was intentional):
a2a5fc1#diff-e87368e6d93ba40ac4d6769c47823b899722ace412def9f508c4fc8ceebd2773R329-R330

I wonder if that is no longer the case, and if there are no other dependencies on the order in all other usages. If that's so, absl::flat_hash_map can definitely be used here.