Skip to content

Commit caa8170

Browse files
hpeebleshpeebles
authored
perf: avoid cloning full network topology in more scenarios (#10618)
This PR follows on from #10594 by wrapping `SystemMetadata::network_topology` in an `Arc` which allows us to avoid deep cloning the full network topology in a few more scenarios: Scenario | Frequency | Location -- | -- | -- Scheduler hands the topology to message execution for the round | per execution round | scheduler.rs:528 → inner_round Building a QueryContext | per query | query_context.rs:151 → QueryContext::new Inspect message | per ingress message | execution_environment.rs:3468 → should_accept_ingress_message Canister install / upgrade | per install/upgrade | execution_environment.rs:4133 → execute_install_code Resumed install / upgrade (DTS continuation) | per resumed slice | execution_environment.rs:4311 → resume_install_code The vast majority of the changes in this PR are simply updating tests to work with the new `Arc` wrapper.
1 parent 6f17d20 commit caa8170

24 files changed

Lines changed: 576 additions & 498 deletions

File tree

‎rs/canonical_state/src/traversal.rs‎

Lines changed: 87 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,10 @@ mod tests {
6464
ExecutionState, ExportedFunctions, NumWasmPages,
6565
execution_state::{CustomSection, CustomSectionType, WasmBinary, WasmMetadata},
6666
},
67-
metadata_state::{ApiBoundaryNodeEntry, SubnetTopology, testing::NetworkTopologyTesting},
67+
metadata_state::{
68+
ApiBoundaryNodeEntry, SubnetTopology,
69+
testing::{NetworkTopologyTesting, SystemMetadataTesting},
70+
},
6871
page_map::PageMap,
6972
testing::{ReplicatedStateTesting, StreamTesting},
7073
};
@@ -766,55 +769,57 @@ mod tests {
766769
fn test_traverse_subnet() {
767770
let mut state = ReplicatedState::new(subnet_test_id(1), SubnetType::Application);
768771

769-
state.metadata.network_topology.set_subnets(btreemap! {
770-
// For test coverage, this test adds one subnet for each subnet type
771-
subnet_test_id(0) => SubnetTopology {
772-
public_key: vec![1, 2, 3, 4],
773-
nodes: BTreeSet::new(),
774-
subnet_type: SubnetType::Application,
775-
subnet_features: SubnetFeatures::default(),
776-
chain_keys_held: BTreeSet::new(),
777-
cost_schedule: CanisterCyclesCostSchedule::Normal,
778-
subnet_admins: BTreeSet::new(),
779-
},
780-
subnet_test_id(1) => SubnetTopology {
781-
public_key: vec![5, 6, 7, 8],
782-
nodes: BTreeSet::new(),
783-
subnet_type: SubnetType::System,
784-
subnet_features: SubnetFeatures::default(),
785-
chain_keys_held: BTreeSet::new(),
786-
cost_schedule: CanisterCyclesCostSchedule::Normal,
787-
subnet_admins: BTreeSet::new(),
788-
},
789-
subnet_test_id(2) => SubnetTopology {
790-
public_key: vec![9, 10, 11, 12],
791-
nodes: BTreeSet::new(),
792-
subnet_type: SubnetType::VerifiedApplication,
793-
subnet_features: SubnetFeatures::default(),
794-
chain_keys_held: BTreeSet::new(),
795-
cost_schedule: CanisterCyclesCostSchedule::Normal,
796-
subnet_admins: BTreeSet::new(),
797-
},
798-
subnet_test_id(3) => SubnetTopology {
799-
public_key: vec![13, 14, 15, 16],
800-
nodes: BTreeSet::new(),
801-
subnet_type: SubnetType::CloudEngine,
802-
subnet_features: SubnetFeatures::default(),
803-
chain_keys_held: BTreeSet::new(),
804-
cost_schedule: CanisterCyclesCostSchedule::Normal,
805-
subnet_admins: BTreeSet::new(),
806-
}
772+
state.metadata.modify_network_topology(|network_topology| {
773+
network_topology.set_subnets(btreemap! {
774+
// For test coverage, this test adds one subnet for each subnet type
775+
subnet_test_id(0) => SubnetTopology {
776+
public_key: vec![1, 2, 3, 4],
777+
nodes: BTreeSet::new(),
778+
subnet_type: SubnetType::Application,
779+
subnet_features: SubnetFeatures::default(),
780+
chain_keys_held: BTreeSet::new(),
781+
cost_schedule: CanisterCyclesCostSchedule::Normal,
782+
subnet_admins: BTreeSet::new(),
783+
},
784+
subnet_test_id(1) => SubnetTopology {
785+
public_key: vec![5, 6, 7, 8],
786+
nodes: BTreeSet::new(),
787+
subnet_type: SubnetType::System,
788+
subnet_features: SubnetFeatures::default(),
789+
chain_keys_held: BTreeSet::new(),
790+
cost_schedule: CanisterCyclesCostSchedule::Normal,
791+
subnet_admins: BTreeSet::new(),
792+
},
793+
subnet_test_id(2) => SubnetTopology {
794+
public_key: vec![9, 10, 11, 12],
795+
nodes: BTreeSet::new(),
796+
subnet_type: SubnetType::VerifiedApplication,
797+
subnet_features: SubnetFeatures::default(),
798+
chain_keys_held: BTreeSet::new(),
799+
cost_schedule: CanisterCyclesCostSchedule::Normal,
800+
subnet_admins: BTreeSet::new(),
801+
},
802+
subnet_test_id(3) => SubnetTopology {
803+
public_key: vec![13, 14, 15, 16],
804+
nodes: BTreeSet::new(),
805+
subnet_type: SubnetType::CloudEngine,
806+
subnet_features: SubnetFeatures::default(),
807+
chain_keys_held: BTreeSet::new(),
808+
cost_schedule: CanisterCyclesCostSchedule::Normal,
809+
subnet_admins: BTreeSet::new(),
810+
}
811+
});
812+
network_topology.set_routing_table(
813+
RoutingTable::try_from(btreemap! {
814+
id_range(0, 10) => subnet_test_id(0),
815+
id_range(11, 20) => subnet_test_id(1),
816+
id_range(21, 30) => subnet_test_id(0),
817+
id_range(31, 40) => subnet_test_id(2),
818+
id_range(41, 50) => subnet_test_id(3),
819+
})
820+
.unwrap(),
821+
);
807822
});
808-
state.metadata.network_topology.set_routing_table(
809-
RoutingTable::try_from(btreemap! {
810-
id_range(0, 10) => subnet_test_id(0),
811-
id_range(11, 20) => subnet_test_id(1),
812-
id_range(21, 30) => subnet_test_id(0),
813-
id_range(31, 40) => subnet_test_id(2),
814-
id_range(41, 50) => subnet_test_id(3),
815-
})
816-
.unwrap(),
817-
);
818823
state.metadata.node_public_keys = btreemap! {
819824
node_test_id(2) => vec![9, 10, 11, 12],
820825
};
@@ -1032,38 +1037,40 @@ mod tests {
10321037
fn test_traverse_large_or_empty_routing_table() {
10331038
let mut state = ReplicatedState::new(subnet_test_id(1), SubnetType::Application);
10341039

1035-
state.metadata.network_topology.set_subnets(btreemap! {
1036-
subnet_test_id(0) => SubnetTopology {
1037-
public_key: vec![1, 2, 3, 4],
1038-
nodes: BTreeSet::new(),
1039-
subnet_type: SubnetType::Application,
1040-
subnet_features: SubnetFeatures::default(),
1041-
chain_keys_held: BTreeSet::new(),
1042-
cost_schedule: CanisterCyclesCostSchedule::Normal,
1043-
subnet_admins: BTreeSet::new(),
1044-
},
1045-
subnet_test_id(1) => SubnetTopology {
1046-
public_key: vec![5, 6, 7, 8],
1047-
nodes: BTreeSet::new(),
1048-
subnet_type: SubnetType::VerifiedApplication,
1049-
subnet_features: SubnetFeatures::default(),
1050-
chain_keys_held: BTreeSet::new(),
1051-
cost_schedule: CanisterCyclesCostSchedule::Normal,
1052-
subnet_admins: BTreeSet::new(),
1053-
}
1040+
state.metadata.modify_network_topology(|network_topology| {
1041+
network_topology.set_subnets(btreemap! {
1042+
subnet_test_id(0) => SubnetTopology {
1043+
public_key: vec![1, 2, 3, 4],
1044+
nodes: BTreeSet::new(),
1045+
subnet_type: SubnetType::Application,
1046+
subnet_features: SubnetFeatures::default(),
1047+
chain_keys_held: BTreeSet::new(),
1048+
cost_schedule: CanisterCyclesCostSchedule::Normal,
1049+
subnet_admins: BTreeSet::new(),
1050+
},
1051+
subnet_test_id(1) => SubnetTopology {
1052+
public_key: vec![5, 6, 7, 8],
1053+
nodes: BTreeSet::new(),
1054+
subnet_type: SubnetType::VerifiedApplication,
1055+
subnet_features: SubnetFeatures::default(),
1056+
chain_keys_held: BTreeSet::new(),
1057+
cost_schedule: CanisterCyclesCostSchedule::Normal,
1058+
subnet_admins: BTreeSet::new(),
1059+
}
1060+
});
1061+
network_topology.set_routing_table(
1062+
RoutingTable::try_from(btreemap! {
1063+
id_range(0, 10) => subnet_test_id(0),
1064+
id_range(21, 30) => subnet_test_id(0),
1065+
id_range(36, 40) => subnet_test_id(0),
1066+
id_range(51, 51) => subnet_test_id(0),
1067+
id_range(61, 70) => subnet_test_id(0),
1068+
id_range(81, 90) => subnet_test_id(0),
1069+
id_range(105, 110) => subnet_test_id(0),
1070+
})
1071+
.unwrap(),
1072+
);
10541073
});
1055-
state.metadata.network_topology.set_routing_table(
1056-
RoutingTable::try_from(btreemap! {
1057-
id_range(0, 10) => subnet_test_id(0),
1058-
id_range(21, 30) => subnet_test_id(0),
1059-
id_range(36, 40) => subnet_test_id(0),
1060-
id_range(51, 51) => subnet_test_id(0),
1061-
id_range(61, 70) => subnet_test_id(0),
1062-
id_range(81, 90) => subnet_test_id(0),
1063-
id_range(105, 110) => subnet_test_id(0),
1064-
})
1065-
.unwrap(),
1066-
);
10671074

10681075
let height = 0;
10691076

‎rs/execution_environment/src/canister_manager/tests.rs‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,8 @@ use ic_replicated_state::{
5656
CallContextManager, CallOrigin, CanisterState, CanisterStatus, ReplicatedState,
5757
canister_state::system_state::wasm_chunk_store::{self, ChunkValidationResult},
5858
metadata_state::{
59-
subnet_call_context_manager::InstallCodeCallId, testing::NetworkTopologyTesting,
59+
subnet_call_context_manager::InstallCodeCallId,
60+
testing::{NetworkTopologyTesting, SystemMetadataTesting},
6061
},
6162
page_map::TestPageAllocatorFileDescriptorImpl,
6263
testing::{CanisterQueuesTesting, SystemStateTesting},
@@ -339,12 +340,10 @@ fn initial_state(subnet_id: SubnetId, use_specified_ids_routing_table: bool) ->
339340
})
340341
.unwrap()
341342
};
342-
state
343-
.metadata
344-
.network_topology
345-
.set_routing_table(routing_table);
346-
347-
state.metadata.network_topology.nns_subnet_id = subnet_id;
343+
state.metadata.modify_network_topology(|network_topology| {
344+
network_topology.set_routing_table(routing_table);
345+
network_topology.nns_subnet_id = subnet_id;
346+
});
348347
state.metadata.init_allocation_ranges_if_empty().unwrap();
349348
state
350349
}
@@ -405,7 +404,7 @@ fn install_code(
405404
None,
406405
old_canister,
407406
time,
408-
Arc::new(network_topology),
407+
network_topology,
409408
execution_parameters,
410409
round_limits,
411410
CompilationCostHandling::CountFullAmount,

‎rs/execution_environment/src/execution/install_code/tests.rs‎

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use ic_replicated_state::{
1414
canister_state::{
1515
NextExecution, execution_state::WasmExecutionMode, system_state::wasm_chunk_store,
1616
},
17-
metadata_state::testing::NetworkTopologyTesting,
17+
metadata_state::testing::{NetworkTopologyTesting, SystemMetadataTesting},
1818
};
1919
use ic_test_utilities_execution_environment::{
2020
ExecutionTest, ExecutionTestBuilder, check_ingress_status, get_reply,
@@ -1205,14 +1205,14 @@ fn subnet_split_cleans_in_progress_install_code_calls() {
12051205
// A no-op subnet split (no canisters migrated).
12061206
test.state_mut()
12071207
.metadata
1208-
.network_topology
1209-
.routing_table_mut()
1210-
.assign_canister(canister_id_1, own_subnet_id);
1211-
test.state_mut()
1212-
.metadata
1213-
.network_topology
1214-
.routing_table_mut()
1215-
.assign_canister(canister_id_2, own_subnet_id);
1208+
.modify_network_topology(|network_topology| {
1209+
network_topology
1210+
.routing_table_mut()
1211+
.assign_canister(canister_id_1, own_subnet_id);
1212+
network_topology
1213+
.routing_table_mut()
1214+
.assign_canister(canister_id_2, own_subnet_id);
1215+
});
12161216
test.online_split_state(own_subnet_id, other_subnet_id);
12171217

12181218
// Retains the `InstallCodeCall` and does not produce a response.
@@ -1228,9 +1228,11 @@ fn subnet_split_cleans_in_progress_install_code_calls() {
12281228
// Simulate a subnet split that migrates canister 1 to another subnet.
12291229
test.state_mut()
12301230
.metadata
1231-
.network_topology
1232-
.routing_table_mut()
1233-
.assign_canister(canister_id_1, other_subnet_id);
1231+
.modify_network_topology(|network_topology| {
1232+
network_topology
1233+
.routing_table_mut()
1234+
.assign_canister(canister_id_1, other_subnet_id);
1235+
});
12341236
test.online_split_state(own_subnet_id, other_subnet_id);
12351237

12361238
// Should have removed the `InstallCodeCall` and produced a reject response.
@@ -1297,9 +1299,11 @@ fn subnet_split_cleans_in_progress_install_code_calls() {
12971299
// Simulate a subnet split that migrates canister 2 to another subnet.
12981300
test.state_mut()
12991301
.metadata
1300-
.network_topology
1301-
.routing_table_mut()
1302-
.assign_canister(canister_id_2, other_subnet_id);
1302+
.modify_network_topology(|network_topology| {
1303+
network_topology
1304+
.routing_table_mut()
1305+
.assign_canister(canister_id_2, other_subnet_id);
1306+
});
13031307
test.online_split_state(own_subnet_id, other_subnet_id);
13041308

13051309
// Should have removed the `InstallCodeCall` and set the ingress state to `Failed`.

‎rs/execution_environment/src/execution_environment.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3465,7 +3465,7 @@ impl ExecutionEnvironment {
34653465
execution_parameters,
34663466
subnet_available_memory,
34673467
&self.hypervisor,
3468-
Arc::new(state.metadata.network_topology.clone()),
3468+
Arc::clone(&state.metadata.network_topology),
34693469
&self.log,
34703470
&self.metrics.state_changes_error,
34713471
metrics,
@@ -4130,7 +4130,7 @@ impl ExecutionEnvironment {
41304130
prepaid_execution_cycles,
41314131
old_canister,
41324132
state.time(),
4133-
Arc::new(state.metadata.network_topology.clone()),
4133+
Arc::clone(&state.metadata.network_topology),
41344134
execution_parameters,
41354135
round_limits,
41364136
compilation_cost_handling,
@@ -4308,7 +4308,7 @@ impl ExecutionEnvironment {
43084308
ingress_with_cycles_error: &self.metrics.ingress_with_cycles_error,
43094309
};
43104310
let round = RoundContext {
4311-
network_topology: Arc::new(state.metadata.network_topology.clone()),
4311+
network_topology: Arc::clone(&state.metadata.network_topology),
43124312
hypervisor: &self.hypervisor,
43134313
cycles_account_manager: &self.cycles_account_manager,
43144314
counters: round_counters,

‎rs/execution_environment/src/execution_environment/tests.rs‎

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ use ic_replicated_state::{
2020
CanisterStatus, ReplicatedState, SystemState,
2121
canister_state::{DEFAULT_QUEUE_CAPACITY, WASM_PAGE_SIZE_IN_BYTES},
2222
metadata_state::subnet_call_context_manager::PreSignatureStash,
23-
metadata_state::testing::NetworkTopologyTesting,
23+
metadata_state::testing::{NetworkTopologyTesting, SystemMetadataTesting},
2424
testing::{CanisterQueuesTesting, SystemStateTesting},
2525
};
2626
use ic_test_utilities::assert_utils::assert_balance_equals;
@@ -1827,14 +1827,14 @@ fn subnet_split_cleans_in_progress_stop_canister_calls() {
18271827
// A no-op subnet split (no canisters migrated).
18281828
test.state_mut()
18291829
.metadata
1830-
.network_topology
1831-
.routing_table_mut()
1832-
.assign_canister(canister_id_1, own_subnet_id);
1833-
test.state_mut()
1834-
.metadata
1835-
.network_topology
1836-
.routing_table_mut()
1837-
.assign_canister(canister_id_2, own_subnet_id);
1830+
.modify_network_topology(|network_topology| {
1831+
network_topology
1832+
.routing_table_mut()
1833+
.assign_canister(canister_id_1, own_subnet_id);
1834+
network_topology
1835+
.routing_table_mut()
1836+
.assign_canister(canister_id_2, own_subnet_id);
1837+
});
18381838
test.online_split_state(own_subnet_id, other_subnet_id);
18391839

18401840
// Retains the `StopCanisterCall` and does not produce a response.
@@ -1850,9 +1850,11 @@ fn subnet_split_cleans_in_progress_stop_canister_calls() {
18501850
// Simulate a subnet split that migrates canister 1 to another subnet.
18511851
test.state_mut()
18521852
.metadata
1853-
.network_topology
1854-
.routing_table_mut()
1855-
.assign_canister(canister_id_1, other_subnet_id);
1853+
.modify_network_topology(|network_topology| {
1854+
network_topology
1855+
.routing_table_mut()
1856+
.assign_canister(canister_id_1, other_subnet_id);
1857+
});
18561858
test.online_split_state(own_subnet_id, other_subnet_id);
18571859

18581860
// Should have removed the `StopCanisterCall` and produced a reject response.
@@ -1908,9 +1910,11 @@ fn subnet_split_cleans_in_progress_stop_canister_calls() {
19081910
// Simulate a subnet split that migrates canister 2 to another subnet.
19091911
test.state_mut()
19101912
.metadata
1911-
.network_topology
1912-
.routing_table_mut()
1913-
.assign_canister(canister_id_2, other_subnet_id);
1913+
.modify_network_topology(|network_topology| {
1914+
network_topology
1915+
.routing_table_mut()
1916+
.assign_canister(canister_id_2, other_subnet_id);
1917+
});
19141918
test.online_split_state(own_subnet_id, other_subnet_id);
19151919

19161920
// Should have removed the `StopCanisterCall` and set the ingress state to `Failed`.

‎rs/execution_environment/src/query_handler/query_context.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ impl<'a> QueryContext<'a> {
148148
cycles_account_manager: Arc<CyclesAccountManager>,
149149
instruction_observation: Option<Arc<AtomicU64>>,
150150
) -> Self {
151-
let network_topology = Arc::new(state.get_ref().metadata.network_topology.clone());
151+
let network_topology = Arc::clone(&state.get_ref().metadata.network_topology);
152152
let round_limits = RoundLimits {
153153
instructions: as_round_instructions(max_query_call_graph_instructions),
154154
subnet_available_memory,

‎rs/execution_environment/src/scheduler.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -525,7 +525,7 @@ impl SchedulerImpl {
525525
active_canisters_partitioned_by_cores,
526526
current_round,
527527
state.time(),
528-
Arc::new(state.metadata.network_topology.clone()),
528+
Arc::clone(&state.metadata.network_topology),
529529
subnet_cycles_config,
530530
&mut round_limits,
531531
state.resource_limits(),

0 commit comments

Comments
 (0)