Remove unneeded uses of indexmap #1048

olix0r · 2021-06-16T15:09:13Z

We initially adopted indexmap in many places where it's not strictly
necessary: we don't actually rely on the data being insert-ordered and
don't need to mutate the structure while iterating over it. In fact, the
traffic split module is the only place in the codebase where we access
the data structure by index (rather than by key).

In all other cases, we do not care about insertion-order. In the vast
majority of cases we can simply use a HashMap. Endpoint and route
labels need to be reliably ordered so that they can be compared (i.e.
implementing Eq), in which case the standard BTreeMap works
sufficiently.

This change eliminates unnecessary uses of indexmap to reduce
dependency and conceptual overhead.

We initially adopted `indexmap` in many places where it's not strictly necessary: we don't actually rely on the data being insert-ordered and don't need to mutate the structure while iterating over it. In fact, the traffic split module is the only place in the codebase where we access the data structure by index (rather than by key). In all other cases, we do not care about insertion-order. In the vast majority of cases we can simply use a `HashMap`. Endpoint and route labels need to be reliably ordered so that they can be compared (i.e. implementing `Eq`), in which case the standard `BTreeMap` works sufficiently. This change eliminates unnecessary uses of `indexmap` to reduce dependency and conceptual overhead.

hawkw · 2021-06-16T16:12:47Z

I was also thinking about doing this yesterday when I was looking at metrics stuff...I suspect that HashMap has actually outperformed IndexMap in a lot of cases since the switch to hashbrown.

codecov-commenter · 2021-06-17T01:08:17Z

Codecov Report

Merging #1048 (bf8d99d) into main (8fa04f5) will decrease coverage by 0.00%.
The diff coverage is 84.84%.

@@            Coverage Diff             @@
##             main    #1048      +/-   ##
==========================================
- Coverage   74.45%   74.45%   -0.01%     
==========================================
  Files         233      233              
  Lines       14671    14669       -2     
==========================================
- Hits        10924    10922       -2     
  Misses       3747     3747

Impacted Files	Coverage Δ
linkerd/app/inbound/src/require_identity.rs	`61.53% <ø> (ø)`
linkerd/app/inbound/src/target.rs	`61.40% <0.00%> (ø)`
linkerd/app/outbound/src/http/mod.rs	`50.00% <0.00%> (ø)`
linkerd/app/src/tap.rs	`75.00% <ø> (ø)`
linkerd/error-metrics/src/lib.rs	`93.33% <ø> (ø)`
linkerd/metrics/src/scopes.rs	`0.00% <0.00%> (ø)`
linkerd/proxy/tap/src/lib.rs	`26.66% <ø> (ø)`
linkerd/stack/metrics/src/lib.rs	`82.05% <ø> (ø)`
linkerd/app/outbound/src/http/endpoint.rs	`94.92% <100.00%> (ø)`
linkerd/app/outbound/src/tcp/opaque_transport.rs	`85.32% <100.00%> (ø)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8fa04f5...bf8d99d. Read the comment docs.

hawkw

overall, this looks good, but i noticed one place where we are still collecting an iterator of labels into a Vec, sorting it, and then collecting the Vec into a BTreeMap...because BTreeMaps are always sorted, we shouldn't have to do that.

once that's fixed, this should be good to merge!

linkerd/app/inbound/src/require_identity.rs

linkerd/proxy/api-resolve/src/metadata.rs

linkerd/proxy/tap/src/lib.rs

linkerd/service-profiles/src/http/mod.rs

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

This branch adds a `PortSet` type that's a simple wrapper around `std::collections::HashSet` but specialized for storing ports (`u16` values). The `PortSet` type overrides the default hasher so that we don't have to actually hash ports, and just use their numeric value. See here for details: #1048 (comment)

hawkw

okay, i made all the minor changes i mentioned (and opened a separate follow-up PR for https://github.com/linkerd/linkerd2-proxy/pull/1048/files#r653855672, PR #1106). this looks good to merge now!

This branch adds a `PortSet` type that's a simple wrapper around `std::collections::HashSet` but specialized for storing ports (`u16` values). The `PortSet` type overrides the default hasher so that we don't have to actually hash ports, and just use their numeric value. See here for details: #1048 (comment)

This release updates a wide variety of the proxy's dependencies. No user-facing changes are expected. --- * tap: Restore tap matching tests (linkerd/linkerd2-proxy#1042) * Add a 'rustfmt' feature to crates that use gRPC (linkerd/linkerd2-proxy#1041) * tap: return an error when a `PortRange`'s min > max (linkerd/linkerd2-proxy#1044) * retry: Enforce a cap on max replay buffer size (linkerd/linkerd2-proxy#1043) * Add codecov integration via cargo-tarpaulin (linkerd/linkerd2-proxy#1047) * Add a dependabot configuration (linkerd/linkerd2-proxy#1046) * Fixup dependabot configuration (linkerd/linkerd2-proxy#1049) * build(deps): bump actions/upload-artifact from 2.2.3 to 2.2.4 (linkerd/linkerd2-proxy#1050) * build(deps): bump softprops/action-gh-release from 0.1.5 to 1 (linkerd/linkerd2-proxy#1051) * build(deps): bump libfuzzer-sys in /linkerd/transport-header/fuzz (linkerd/linkerd2-proxy#1052) * build(deps): bump libfuzzer-sys from 0.4.0 to 0.4.2 in /linkerd/tls/fuzz (linkerd/linkerd2-proxy#1053) * build(deps): bump libfuzzer-sys in /linkerd/addr/fuzz (linkerd/linkerd2-proxy#1054) * build(deps): bump tokio from 1.5.0 to 1.7.0 in /linkerd/proxy/http/fuzz (linkerd/linkerd2-proxy#1056) * build(deps): bump tracing from 0.1.25 to 0.1.26 in /linkerd/dns/fuzz (linkerd/linkerd2-proxy#1055) * build(deps): bump libfuzzer-sys from 0.4.0 to 0.4.2 in /linkerd/dns/fuzz (linkerd/linkerd2-proxy#1058) * build(deps): bump tracing from 0.1.25 to 0.1.26 in /linkerd/addr/fuzz (linkerd/linkerd2-proxy#1059) * build(deps): bump libfuzzer-sys in /linkerd/proxy/http/fuzz (linkerd/linkerd2-proxy#1060) * build(deps): bump tracing in /linkerd/transport-header/fuzz (linkerd/linkerd2-proxy#1061) * build(deps): bump tokio from 1.5.0 to 1.7.0 in /linkerd/dns/fuzz (linkerd/linkerd2-proxy#1062) * build(deps): bump tokio in /linkerd/transport-header/fuzz (linkerd/linkerd2-proxy#1063) * build(deps): bump arbitrary in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1064) * build(deps): bump tokio from 1.5.0 to 1.7.0 in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1065) * build(deps): bump tracing in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1066) * build(deps): bump hyper in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1067) * build(deps): bump deflate from 0.7.20 to 0.9.1 (linkerd/linkerd2-proxy#1068) * build(deps): bump libfuzzer-sys in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1069) * build(deps): bump tonic-build from 0.4.0 to 0.4.2 (linkerd/linkerd2-proxy#1070) * build(deps): bump linkerd2-proxy-api from `453ac1e` to `48f13d6` (linkerd/linkerd2-proxy#1071) * build(deps): bump ipnet from 2.3.0 to 2.3.1 (linkerd/linkerd2-proxy#1072) * build(deps): bump regex from 1.4.3 to 1.4.6 (linkerd/linkerd2-proxy#1073) * build(deps): bump arbitrary in /linkerd/transport-header/fuzz (linkerd/linkerd2-proxy#1057) * Update tonic to v0.4.3 (linkerd/linkerd2-proxy#1080) * ci: Skip coverage tests on dependencies-only changes (linkerd/linkerd2-proxy#1074) * build(deps): bump indexmap from 1.6.1 to 1.6.2 (linkerd/linkerd2-proxy#1075) * build(deps): bump rand from 0.8.3 to 0.8.4 (linkerd/linkerd2-proxy#1077) * build(deps): bump tokio-test from 0.4.0 to 0.4.2 (linkerd/linkerd2-proxy#1076) * build(deps): bump pin-project from 1.0.5 to 1.0.7 (linkerd/linkerd2-proxy#1078) * build(deps): bump arbitrary from 1.0.0 to 1.0.1 (linkerd/linkerd2-proxy#1083) * outbound: replace `sleep`s in test with `yield_now` (linkerd/linkerd2-proxy#1085) * build(deps): bump thiserror from 1.0.23 to 1.0.25 (linkerd/linkerd2-proxy#1081) * build(deps): bump async-stream from 0.3.0 to 0.3.2 (linkerd/linkerd2-proxy#1084) * build(deps): bump serde_json from 1.0.62 to 1.0.64 (linkerd/linkerd2-proxy#1086) * build(deps): bump tokio-stream from 0.1.5 to 0.1.6 (linkerd/linkerd2-proxy#1082) * build(deps): Skip coverage by PR title (linkerd/linkerd2-proxy#1087) * fuzz: Omit lockfiles from version control (linkerd/linkerd2-proxy#1088) * build(deps): bump tokio from 1.6.1 to 1.7.0 (linkerd/linkerd2-proxy#1079) * build(deps): bump mimalloc from 0.1.24 to 0.1.25 (linkerd/linkerd2-proxy#1089) * build(deps): bump tokio-util from 0.6.5 to 0.6.7 (linkerd/linkerd2-proxy#1090) * build(deps): bump libc from 0.2.86 to 0.2.97 (linkerd/linkerd2-proxy#1093) * build(deps): bump http from 0.2.3 to 0.2.4 (linkerd/linkerd2-proxy#1092) * build(deps): bump rustls from 0.19.0 to 0.19.1 (linkerd/linkerd2-proxy#1094) * build(deps): bump async-trait from 0.1.42 to 0.1.50 (linkerd/linkerd2-proxy#1095) * build(deps): bump tracing-subscriber from 0.2.17 to 0.2.18 (linkerd/linkerd2-proxy#1096) * build(deps): bump http-body from 0.4.0 to 0.4.2 (linkerd/linkerd2-proxy#1097) * build(deps): bump tracing from 0.1.25 to 0.1.26 (linkerd/linkerd2-proxy#1098) * build(deps): bump hdrhistogram from 7.2.0 to 7.3.0 (linkerd/linkerd2-proxy#1099) * build(deps): bump regex from 1.4.6 to 1.5.4 (linkerd/linkerd2-proxy#1100) * build(deps): bump tower from 0.4.7 to 0.4.8 (linkerd/linkerd2-proxy#1102) * build(deps): bump libfuzzer-sys from 0.4.0 to 0.4.2 (linkerd/linkerd2-proxy#1103) * build(deps): bump actions/download-artifact from 2.0.9 to 2.0.10 (linkerd/linkerd2-proxy#1104) * Remove unneeded uses of indexmap (linkerd/linkerd2-proxy#1048) * build(deps): Update trust-dns to v0.21.0-alpha.1 (linkerd/linkerd2-proxy#1105) * config: add a `PortSet` type (linkerd/linkerd2-proxy#1106) * replace `linkerd-concurrency-limit` with Tower (linkerd/linkerd2-proxy#1107) * build(deps): Update Rust to v1.53.0 (linkerd/linkerd2-proxy#1108)

olix0r added 7 commits June 16, 2021 17:48

fix dns suffix tests

6460f2f

Merge branch 'main' into ver/no-indexmap

6f9c4a8

ci: Skip coverage tests on dependencies-only changes

6c41726

Merge branch 'main' into ver/no-indexmap

77ff851

update fuzz lockfiles

7177a2c

Merge branch 'main' into ver/no-indexmap

4e0cbc9

Merge branch 'main' into ver/no-indexmap

b6ff79b

olix0r marked this pull request as ready for review June 17, 2021 01:06

olix0r requested a review from a team June 17, 2021 01:06

Remove unneeded indexmap dep

bf8d99d

hawkw suggested changes Jun 17, 2021

View reviewed changes

linkerd/app/inbound/src/require_identity.rs Show resolved Hide resolved

linkerd/proxy/api-resolve/src/metadata.rs Show resolved Hide resolved

linkerd/proxy/tap/src/lib.rs Show resolved Hide resolved

linkerd/service-profiles/src/http/mod.rs Outdated Show resolved Hide resolved

hawkw added 2 commits June 17, 2021 12:33

service-profiles: remove unneeded sort

231af89

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

be the comments you wish to see in the world

21b05b9

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw mentioned this pull request Jun 17, 2021

config: add a PortSet type #1106

Merged

hawkw approved these changes Jun 17, 2021

View reviewed changes

hawkw merged commit cde2816 into main Jun 17, 2021

hawkw deleted the ver/no-indexmap branch June 17, 2021 20:25

olix0r mentioned this pull request Jun 18, 2021

proxy: v2.148.0 linkerd/linkerd2#6314

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unneeded uses of indexmap #1048

Remove unneeded uses of indexmap #1048

olix0r commented Jun 16, 2021

hawkw commented Jun 16, 2021

codecov-commenter commented Jun 17, 2021 •

edited

Loading

hawkw left a comment

hawkw left a comment

Remove unneeded uses of indexmap #1048

Remove unneeded uses of indexmap #1048

Conversation

olix0r commented Jun 16, 2021

hawkw commented Jun 16, 2021

codecov-commenter commented Jun 17, 2021 • edited Loading

Codecov Report

hawkw left a comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 17, 2021 •

edited

Loading