Skip to content

Commit

Permalink
feat(nns): Avoid cloning large fields when listing proposals (#3505)
Browse files Browse the repository at this point in the history 
# Why

When converting from `ProposalData` to `ProposalInfo`, the large fields
(e.g. canister WASMs, logos) are "omitted" in a way that those fields
are first cloned then cleared. It indeed saves space in the response by
not including them, but the instructions are nevertheless wasted for the
clone, and this can be significant for certain queries (e.g. listing
only proposals including canister WASMs).

# What

* Make `ProposalInfo`/`ListProposalInfoResponse` pure API types, by
removing its corresponding internal type
* Add conversion method `proposal_data_to_info` from the `ProposalData`
internal type to `ProposalInfo` API type
* Implement the equivalent behavior of `omit_large_fields` in the
conversion code
* Let `Governance::list_proposals` use the new conversion method and
return `ListProposalInfoResponse`(API type)
* Update benchmark results (98% improvement on list_proposals)

# Tests

`rs/nns/governance/tests/governance.rs` already tests various cases for
`omit_large_fields`
  • Loading branch information
@jasonz-dfinity
jasonz-dfinity authored Jan 28, 2025
1 parent aa12bb1 commit 3aa3266
Show file tree
Hide file tree
Showing 32 changed files with 596 additions and 741 deletions.
27 changes: 2 additions & 25 deletions rs/nns/governance/api/src/ic_nns_governance.pb.v1.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2158,61 +2158,41 @@ pub struct WaitForQuietState {
/// This is a view of the ProposalData returned by API queries and is NOT used
/// for storage. The ballots are restricted to those of the caller's neurons and
/// additionally it has the computed fields, topic, status, and reward_status.
#[derive(candid::CandidType, candid::Deserialize, serde::Serialize, comparable::Comparable)]
#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Clone, PartialEq, ::prost::Message)]
#[derive(candid::CandidType, candid::Deserialize, serde::Serialize, Clone, Debug, PartialEq)]
pub struct ProposalInfo {
/// The unique id for this proposal.
#[prost(message, optional, tag = "1")]
pub id: Option<::ic_nns_common::pb::v1::ProposalId>,
/// The ID of the neuron that made this proposal.
#[prost(message, optional, tag = "2")]
pub proposer: Option<NeuronId>,
/// The amount of ICP in E8s to be charged to the proposer if the proposal is
/// rejected.
#[prost(uint64, tag = "3")]
pub reject_cost_e8s: u64,
/// The proposal originally submitted.
#[prost(message, optional, tag = "4")]
pub proposal: Option<Proposal>,
/// The timestamp, in seconds from the Unix epoch, when this proposal was made.
#[prost(uint64, tag = "5")]
pub proposal_timestamp_seconds: u64,
/// See \[ProposalData::ballots\].
#[prost(map = "fixed64, message", tag = "6")]
pub ballots: ::std::collections::HashMap<u64, Ballot>,
/// See \[ProposalData::latest_tally\].
#[prost(message, optional, tag = "7")]
pub latest_tally: Option<Tally>,
/// See \[ProposalData::decided_timestamp_seconds\].
#[prost(uint64, tag = "8")]
pub decided_timestamp_seconds: u64,
/// See \[ProposalData::executed_timestamp_seconds\].
#[prost(uint64, tag = "12")]
pub executed_timestamp_seconds: u64,
/// See \[ProposalData::failed_timestamp_seconds\].
#[prost(uint64, tag = "13")]
pub failed_timestamp_seconds: u64,
/// See \[ProposalData::failure_reason\].
#[prost(message, optional, tag = "18")]
pub failure_reason: Option<GovernanceError>,
/// See \[ProposalData::reward_event_round\].
#[prost(uint64, tag = "14")]
pub reward_event_round: u64,
/// Derived - see \[Topic\] for more information
#[prost(enumeration = "Topic", tag = "15")]
pub topic: i32,
/// Derived - see \[ProposalStatus\] for more information
#[prost(enumeration = "ProposalStatus", tag = "16")]
pub status: i32,
/// Derived - see \[ProposalRewardStatus\] for more information
#[prost(enumeration = "ProposalRewardStatus", tag = "17")]
pub reward_status: i32,
#[prost(uint64, optional, tag = "19")]
pub deadline_timestamp_seconds: Option<u64>,
#[prost(message, optional, tag = "20")]
pub derived_proposal_information: Option<DerivedProposalInformation>,
#[prost(uint64, optional, tag = "21")]
pub total_potential_voting_power: ::core::option::Option<u64>,
}

Expand Down Expand Up @@ -3479,11 +3459,8 @@ pub struct ListProposalInfo {
#[prost(bool, optional, tag = "7")]
pub omit_large_fields: Option<bool>,
}
#[derive(candid::CandidType, candid::Deserialize, serde::Serialize, comparable::Comparable)]
#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Clone, PartialEq, ::prost::Message)]
#[derive(candid::CandidType, candid::Deserialize, serde::Serialize, Clone, Debug, PartialEq)]
pub struct ListProposalInfoResponse {
#[prost(message, repeated, tag = "1")]
pub proposal_info: Vec<ProposalInfo>,
}
/// A request to list neurons. The "requested list", i.e., the list of
Expand Down
40 changes: 20 additions & 20 deletions rs/nns/governance/canbench/canbench_results.yml
View file
Original file line number Diff line number Diff line change
@@ -1,67 +1,67 @@
benches:
add_neuron_active_maximum:
total:
instructions: 42749552
instructions: 42752796
heap_increase: 1
stable_memory_increase: 0
scopes: {}
add_neuron_active_typical:
total:
instructions: 2170522
instructions: 2170658
heap_increase: 0
stable_memory_increase: 0
scopes: {}
add_neuron_inactive_maximum:
total:
instructions: 112621390
instructions: 112624375
heap_increase: 1
stable_memory_increase: 0
scopes: {}
add_neuron_inactive_typical:
total:
instructions: 8497159
instructions: 8497036
heap_increase: 0
stable_memory_increase: 0
scopes: {}
cascading_vote_all_heap:
total:
instructions: 35650914
instructions: 35676146
heap_increase: 0
stable_memory_increase: 128
scopes: {}
cascading_vote_heap_neurons_stable_index:
total:
instructions: 61785953
instructions: 61811185
heap_increase: 0
stable_memory_increase: 128
scopes: {}
cascading_vote_stable_everything:
total:
instructions: 188618691
instructions: 188611915
heap_increase: 0
stable_memory_increase: 128
scopes: {}
cascading_vote_stable_neurons_with_heap_index:
total:
instructions: 162350256
instructions: 162343480
heap_increase: 0
stable_memory_increase: 128
scopes: {}
centralized_following_all_stable:
total:
instructions: 78266711
instructions: 78265237
heap_increase: 0
stable_memory_increase: 128
scopes: {}
compute_ballots_for_new_proposal_with_stable_neurons:
total:
instructions: 2223231
instructions: 2223431
heap_increase: 0
stable_memory_increase: 0
scopes: {}
draw_maturity_from_neurons_fund_heap:
total:
instructions: 7643198
instructions: 7656798
heap_increase: 0
stable_memory_increase: 0
scopes: {}
Expand All @@ -85,7 +85,7 @@ benches:
scopes: {}
list_neurons_heap:
total:
instructions: 4947095
instructions: 4988540
heap_increase: 9
stable_memory_increase: 0
scopes: {}
Expand All @@ -103,13 +103,13 @@ benches:
scopes: {}
list_neurons_stable:
total:
instructions: 113659672
instructions: 113606723
heap_increase: 5
stable_memory_increase: 0
scopes: {}
list_proposals:
total:
instructions: 6477035
instructions: 128658
heap_increase: 0
stable_memory_increase: 0
scopes: {}
Expand All @@ -127,37 +127,37 @@ benches:
scopes: {}
neuron_data_validation_heap:
total:
instructions: 406842391
instructions: 406853184
heap_increase: 0
stable_memory_increase: 0
scopes: {}
neuron_data_validation_stable:
total:
instructions: 362640286
instructions: 362648372
heap_increase: 0
stable_memory_increase: 0
scopes: {}
neuron_metrics_calculation_heap:
total:
instructions: 1498269
instructions: 1498869
heap_increase: 0
stable_memory_increase: 0
scopes: {}
neuron_metrics_calculation_stable:
total:
instructions: 3026795
instructions: 3027495
heap_increase: 0
stable_memory_increase: 0
scopes: {}
range_neurons_performance:
total:
instructions: 56448740
instructions: 56447340
heap_increase: 0
stable_memory_increase: 0
scopes: {}
single_vote_all_stable:
total:
instructions: 2805835
instructions: 2805871
heap_increase: 0
stable_memory_increase: 128
scopes: {}
Expand Down
12 changes: 3 additions & 9 deletions rs/nns/governance/canister/canister.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -774,9 +774,7 @@ fn get_neuron_info_by_id_or_subaccount(
#[query]
fn get_proposal_info(id: ProposalId) -> Option<ProposalInfo> {
debug_log("get_proposal_info");
governance()
.get_proposal_info(&caller(), id)
.map(ProposalInfo::from)
GOVERNANCE.with_borrow(|governance| governance.get_proposal_info(&caller(), id))
}

#[query]
Expand All @@ -792,17 +790,13 @@ fn get_neurons_fund_audit_info(
#[query]
fn get_pending_proposals() -> Vec<ProposalInfo> {
debug_log("get_pending_proposals");
governance()
.get_pending_proposals(&caller())
.into_iter()
.map(ProposalInfo::from)
.collect()
GOVERNANCE.with_borrow(|governance| governance.get_pending_proposals(&caller()))
}

#[query]
fn list_proposals(req: ListProposalInfo) -> ListProposalInfoResponse {
debug_log("list_proposals");
governance().list_proposals(&caller(), &(req.into())).into()
GOVERNANCE.with_borrow(|governance| governance.list_proposals(&caller(), &req.into()))
}

#[query]
Expand Down
62 changes: 0 additions & 62 deletions rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1642,64 +1642,6 @@ message WaitForQuietState {
uint64 current_deadline_timestamp_seconds = 1;
}

// This is a view of the ProposalData returned by API queries and is NOT used
// for storage. The ballots are restricted to those of the caller's neurons and
// additionally it has the computed fields, topic, status, and reward_status.
message ProposalInfo {
// The unique id for this proposal.
ic_nns_common.pb.v1.ProposalId id = 1;

// The ID of the neuron that made this proposal.
ic_nns_common.pb.v1.NeuronId proposer = 2;

// The amount of ICP in E8s to be charged to the proposer if the proposal is
// rejected.
uint64 reject_cost_e8s = 3;

// The proposal originally submitted.
Proposal proposal = 4;

// The timestamp, in seconds from the Unix epoch, when this proposal was made.
uint64 proposal_timestamp_seconds = 5;

// See [ProposalData::ballots].
map<fixed64, Ballot> ballots = 6;

// See [ProposalData::latest_tally].
Tally latest_tally = 7;

// See [ProposalData::decided_timestamp_seconds].
uint64 decided_timestamp_seconds = 8;

// See [ProposalData::executed_timestamp_seconds].
uint64 executed_timestamp_seconds = 12;

// See [ProposalData::failed_timestamp_seconds].
uint64 failed_timestamp_seconds = 13;

// See [ProposalData::failure_reason].
GovernanceError failure_reason = 18;

// See [ProposalData::reward_event_round].
uint64 reward_event_round = 14;

// Derived - see [Topic] for more information
Topic topic = 15;

// Derived - see [ProposalStatus] for more information
ProposalStatus status = 16;

// Derived - see [ProposalRewardStatus] for more information
ProposalRewardStatus reward_status = 17;

optional uint64 deadline_timestamp_seconds = 19;

DerivedProposalInformation derived_proposal_information = 20;

// See [ProposalData::total_potential_voting_power].
optional uint64 total_potential_voting_power = 21;
}

// Network economics contains the parameters for several operations related
// to the economy of the network. When submitting a NetworkEconomics proposal
// default values (0) are considered unchanged, so a valid proposal only needs
Expand Down Expand Up @@ -2492,10 +2434,6 @@ message ListProposalInfo {
optional bool omit_large_fields = 7;
}

message ListProposalInfoResponse {
repeated ProposalInfo proposal_info = 1;
}

// A response to "ListKnownNeurons"
message ListKnownNeuronsResponse {
// List of known neurons.
Expand Down
Loading

0 comments on commit 3aa3266

Please sign in to comment.