Skip to content
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

Chore: Replace Contains Key with Entry #4483

Open
wants to merge 20 commits into
base: next
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
33a9aa8
feat: replace `.contains_key()` with `.entry()` where possible
ASuciuX Mar 4, 2024
6fc8d67
feat: fix formatting errors from `cargo fmt`
ASuciuX Mar 4, 2024
e4b2f8a
feat: uncomment jemalloc
ASuciuX Mar 4, 2024
86cf2a1
replace contains_key in immutable data with raw_entry
ASuciuX Mar 4, 2024
4d8b2f1
removed `raw_entry().from_key(name)` in favor of `contains_key` for b…
ASuciuX Mar 5, 2024
e9d625d
update format
ASuciuX Mar 5, 2024
885b6e5
Merge branch 'next' into chore/replace-contains-key-with-entry
ASuciuX Mar 5, 2024
a8597f1
feat: replace inserts on maps with inserts on entries
ASuciuX Mar 8, 2024
3352be9
feat: code optimizations
ASuciuX Mar 10, 2024
2f14dbd
feat: small code optimization
ASuciuX Mar 11, 2024
a2f924a
feat: remove entry API from all crates besides 'clarity'
ASuciuX Mar 15, 2024
bc2e4c5
feat: remove unused entry import
ASuciuX Mar 15, 2024
c6a0f9f
Merge branch 'next' into chore/replace-contains-key-with-entry
ASuciuX Mar 15, 2024
af75e54
Merge branch 'next' into chore/replace-contains-key-with-entry
jbencin Mar 20, 2024
7434810
Merge branch 'next' into chore/replace-contains-key-with-entry
jbencin Mar 20, 2024
0d914b3
Merge branch 'next' into chore/replace-contains-key-with-entry
jbencin Mar 21, 2024
ab87986
feat: remove `.collect()` from trait definition iter
ASuciuX Mar 22, 2024
283f46b
Merge branch 'next' into chore/replace-contains-key-with-entry
ASuciuX Mar 22, 2024
9055eb0
Merge branch 'next' into chore/replace-contains-key-with-entry
ASuciuX Apr 12, 2024
6526182
add entry trait implementation for readability
ASuciuX Apr 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
feat: replace .contains_key() with .entry() where possible
  • Loading branch information
ASuciuX committed Mar 4, 2024
commit 33a9aa8d3b947cae9b601359712ff27566daf367
11 changes: 4 additions & 7 deletions clarity/src/vm/ast/traits_resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
// You should have received a copy of the GNU General Public License
// along with this program. If not, see <http://www.gnu.org/licenses/>.

use hashbrown::{HashMap, HashSet};
use hashbrown::{HashMap, HashSet, hash_map::Entry};

use crate::vm::analysis::AnalysisDatabase;
use crate::vm::ast::errors::{ParseError, ParseErrors, ParseResult};
Expand Down Expand Up @@ -65,7 +65,7 @@ impl TraitsResolver {
match (&args[0].pre_expr, &args[1].pre_expr) {
(Atom(trait_name), List(trait_definition)) => {
// Check for collisions
if contract_ast.referenced_traits.contains_key(trait_name) {
if let Entry::Occupied(_) = contract_ast.referenced_traits.entry(trait_name.to_owned()) {
return Err(
ParseErrors::NameAlreadyUsed(trait_name.to_string()).into()
);
Expand Down Expand Up @@ -96,7 +96,7 @@ impl TraitsResolver {

if let Some(trait_name) = args[0].match_atom() {
// Check for collisions
if contract_ast.referenced_traits.contains_key(trait_name) {
if let Entry::Occupied(_) = contract_ast.referenced_traits.entry(trait_name.to_owned()) {
return Err(ParseErrors::NameAlreadyUsed(trait_name.to_string()).into());
}

Expand Down Expand Up @@ -161,10 +161,7 @@ impl TraitsResolver {
}

for (trait_reference, expr) in referenced_traits {
if !contract_ast
.referenced_traits
.contains_key(&trait_reference)
{
if let Entry::Vacant(_) = contract_ast.referenced_traits.entry(trait_reference.to_owned()) {
let mut err = ParseError::new(ParseErrors::TraitReferenceUnknown(
trait_reference.to_string(),
));
Expand Down
5 changes: 3 additions & 2 deletions clarity/src/vm/costs/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use std::collections::BTreeMap;
use std::{cmp, fmt};

use hashbrown::hash_map::Entry;
use hashbrown::HashMap;
use lazy_static::lazy_static;
use rusqlite::types::{FromSql, FromSqlResult, ToSql, ToSqlOutput, ValueRef};
Expand Down Expand Up @@ -817,7 +818,7 @@ impl TrackerData {
let cost_function_ref = cost_function_references.remove(f).unwrap_or_else(|| {
ClarityCostFunctionReference::new(boot_costs_id.clone(), f.get_name())
});
if !cost_contracts.contains_key(&cost_function_ref.contract_id) {
if let Entry::Vacant(_) = cost_contracts.entry(cost_function_ref.contract_id.to_owned()) {
let contract_context = match clarity_db.get_contract(&cost_function_ref.contract_id)
{
Ok(contract) => contract.contract_context,
Expand All @@ -838,7 +839,7 @@ impl TrackerData {
}

for (_, circuit_target) in self.contract_call_circuits.iter() {
if !cost_contracts.contains_key(&circuit_target.contract_id) {
if let Entry::Vacant(_) = cost_contracts.entry(circuit_target.contract_id.to_owned()) {
let contract_context = match clarity_db.get_contract(&circuit_target.contract_id) {
Ok(contract) => contract.contract_context,
Err(e) => {
Expand Down
10 changes: 9 additions & 1 deletion clarity/src/vm/database/key_value_wrapper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

use std::hash::Hash;

use hashbrown::hash_map::Entry;
ASuciuX marked this conversation as resolved.
Show resolved Hide resolved
use hashbrown::HashMap;
use stacks_common::types::chainstate::StacksBlockId;
use stacks_common::types::StacksEpochId;
Expand Down Expand Up @@ -541,7 +542,14 @@ impl<'a> RollbackWrapper<'a> {
"ERROR: Clarity VM attempted GET on non-nested context.".into(),
)
})?;
if self.query_pending_data && self.lookup_map.contains_key(key) {

ASuciuX marked this conversation as resolved.
Show resolved Hide resolved
let contains_key = if let Entry::Occupied(_) = self.lookup_map.entry(key.to_owned()) {
true
} else {
false
};

if self.query_pending_data && contains_key {
Ok(true)
} else {
self.store.has_entry(key)
Expand Down
10 changes: 5 additions & 5 deletions clarity/src/vm/types/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ pub mod serialization;
#[allow(clippy::result_large_err)]
pub mod signatures;

use std::collections::btree_map::Entry;
use std::collections::BTreeMap;
use std::{char, cmp, fmt, str};

Expand Down Expand Up @@ -1527,11 +1528,10 @@ impl TupleData {
let mut data_map = BTreeMap::new();
for (name, value) in data.into_iter() {
let type_info = TypeSignature::type_of(&value)?;
if type_map.contains_key(&name) {
return Err(CheckErrors::NameAlreadyUsed(name.into()).into());
} else {
type_map.insert(name.clone(), type_info);
}
match type_map.entry(name.clone()) {
Entry::Vacant(e) => e.insert(type_info),
Entry::Occupied(_) => return Err(CheckErrors::NameAlreadyUsed(name.into()).into()),
};
data_map.insert(name, value);
}

Expand Down
3 changes: 2 additions & 1 deletion libsigner/src/http.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use std::io;
use std::io::{Read, Write};
use std::net::SocketAddr;

use hashbrown::hash_map::Entry;
use hashbrown::HashMap;
use stacks_common::codec::MAX_MESSAGE_LEN;
use stacks_common::deps_common::httparse;
Expand Down Expand Up @@ -179,7 +180,7 @@ pub fn decode_http_response(payload: &[u8]) -> Result<(HashMap<String, String>,
}

let key = resp.headers[i].name.to_string().to_lowercase();
if headers.contains_key(&key) {
if let Entry::Occupied(_) = headers.entry(key.to_owned()) {
return Err(RPCError::MalformedResponse(format!(
"Invalid HTTP respuest: duplicate header \"{}\"",
key
Expand Down
2 changes: 1 addition & 1 deletion stackslib/src/chainstate/stacks/db/blocks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3087,7 +3087,7 @@ impl StacksChainState {
let mut parent_hashes: HashMap<BlockHeaderHash, StacksMicroblockHeader> = HashMap::new();
for i in 0..signed_microblocks.len() {
let signed_microblock = &signed_microblocks[i];
if parent_hashes.contains_key(&signed_microblock.header.prev_block) {
if let std::collections::hash_map::Entry::Occupied(_) = parent_hashes.entry(signed_microblock.header.prev_block.to_owned()) {
debug!(
"Deliberate microblock fork: duplicate parent {}",
signed_microblock.header.prev_block
Expand Down
7 changes: 6 additions & 1 deletion stackslib/src/chainstate/stacks/index/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,12 @@ impl<T: MarfTrieId> TrieCacheState<T> {

/// Cache a block hash, given its ID
pub fn store_block_hash(&mut self, block_id: u32, block_hash: T) {
assert!(!self.block_hash_cache.contains_key(&block_id));
let contains_key = if let std::collections::hash_map::Entry::Occupied(_) = self.block_hash_cache.entry(block_id) {
true
} else {
false
};
assert!(!contains_key);
self.block_id_cache.insert(block_hash.clone(), block_id);
self.block_hash_cache.insert(block_id, block_hash);
}
Expand Down
10 changes: 5 additions & 5 deletions stackslib/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,12 @@ extern crate stacks_common;
#[macro_use(o, slog_log, slog_trace, slog_debug, slog_info, slog_warn, slog_error)]
extern crate slog;

#[cfg(not(target_env = "msvc"))]
use tikv_jemallocator::Jemalloc;
// #[cfg(not(target_env = "msvc"))]
// use tikv_jemallocator::Jemalloc;

#[cfg(not(target_env = "msvc"))]
#[global_allocator]
static GLOBAL: Jemalloc = Jemalloc;
// #[cfg(not(target_env = "msvc"))]
// #[global_allocator]
// static GLOBAL: Jemalloc = Jemalloc;

use std::collections::{HashMap, HashSet};
use std::fs::{File, OpenOptions};
Expand Down
8 changes: 5 additions & 3 deletions stackslib/src/net/dns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
// You should have received a copy of the GNU General Public License
// along with this program. If not, see <http://www.gnu.org/licenses/>.

use std::collections::hash_map::Entry;
use std::collections::{HashMap, HashSet, VecDeque};
use std::hash::{Hash, Hasher};
use std::net::{SocketAddr, ToSocketAddrs};
Expand Down Expand Up @@ -292,7 +293,7 @@ impl DNSClient {
loop {
match self.requests_rx.try_recv() {
Ok(resp) => {
if self.requests.contains_key(&resp.request) {
if let Entry::Occupied(_) = self.requests.entry(resp.request.to_owned()) {
if !resp.request.is_timed_out() {
self.requests.insert(resp.request.clone(), Some(resp));
num_recved += 1;
Expand Down Expand Up @@ -324,7 +325,7 @@ impl DNSClient {

pub fn poll_lookup(&mut self, host: &str, port: u16) -> Result<Option<DNSResponse>, net_error> {
let req = DNSRequest::new(host.to_string(), port, 0);
if !self.requests.contains_key(&req) {
if let Entry::Vacant(_) = self.requests.entry(req.to_owned()) {
return Err(net_error::LookupError(format!(
"No such pending lookup: {}:{}",
host, port
Expand Down Expand Up @@ -357,6 +358,7 @@ impl DNSClient {

#[cfg(test)]
mod test {
use std::collections::hash_map::Entry;
use std::collections::HashMap;
use std::error::Error;

Expand Down Expand Up @@ -423,7 +425,7 @@ mod test {
client.try_recv().unwrap();

for name in names.iter() {
if resolved_addrs.contains_key(&name.to_string()) {
if let Entry::Occupied(_) = resolved_addrs.entry(name.to_string()) {
continue;
}
match client.poll_lookup(name, 80).unwrap() {
Expand Down
9 changes: 5 additions & 4 deletions stackslib/src/net/download.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
// You should have received a copy of the GNU General Public License
// along with this program. If not, see <http://www.gnu.org/licenses/>.

use std::collections::hash_map::Entry;
use std::collections::{HashMap, HashSet, VecDeque};
use std::hash::{Hash, Hasher};
use std::io::{Read, Write};
Expand Down Expand Up @@ -1690,12 +1691,12 @@ impl PeerNetwork {
&& (downloader.blocks_to_try.len() as u64)
< downloader.max_inflight_requests
{
if !next_blocks_to_try.contains_key(&height) {
if let Entry::Vacant(_) = next_blocks_to_try.entry(height) {
height += 1;
continue;
}

if downloader.blocks_to_try.contains_key(&height) {
if let Entry::Occupied(_) = downloader.blocks_to_try.entry(height) {
debug!("Block download already in-flight for {}", height);
height += 1;
continue;
Expand Down Expand Up @@ -1749,12 +1750,12 @@ impl PeerNetwork {
&& (downloader.microblocks_to_try.len() as u64)
< downloader.max_inflight_requests
{
if !next_microblocks_to_try.contains_key(&mblock_height) {
if let Entry::Vacant(_) = next_microblocks_to_try.entry(mblock_height) {
mblock_height += 1;
continue;
}

if downloader.microblocks_to_try.contains_key(&mblock_height) {
if let Entry::Occupied(_) = downloader.microblocks_to_try.entry(mblock_height) {
mblock_height += 1;
debug!(
"Microblocks download already in-flight for {}",
Expand Down
3 changes: 2 additions & 1 deletion stackslib/src/net/inv/nakamoto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
// You should have received a copy of the GNU General Public License
// along with this program. If not, see <http://www.gnu.org/licenses/>.

use std::collections::btree_map::Entry;
use std::collections::{BTreeMap, HashMap};

use stacks_common::bitvec::BitVec;
Expand Down Expand Up @@ -546,7 +547,7 @@ impl<NC: NeighborComms> NakamotoInvStateMachine<NC> {
.expect("FATAL: snapshot occurred before system start");

for rc in highest_rc..=tip_rc {
if self.reward_cycle_consensus_hashes.contains_key(&rc) {
if let Entry::Occupied(_) = self.reward_cycle_consensus_hashes.entry(rc) {
continue;
}
let Some(ch) = Self::load_consensus_hash_for_reward_cycle(sortdb, rc)? else {
Expand Down
29 changes: 21 additions & 8 deletions stackslib/src/net/p2p.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
// along with this program. If not, see <http://www.gnu.org/licenses/>.

use std::cmp::Ordering;
use std::collections::hash_map::Entry;
use std::collections::{HashMap, HashSet, VecDeque};
use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr};
use std::sync::mpsc::{
Expand Down Expand Up @@ -1691,8 +1692,20 @@ impl PeerNetwork {
&self.local_peer, &client_addr, event_id, &neighbor_key, outbound
);

assert!(!self.sockets.contains_key(&event_id));
assert!(!self.peers.contains_key(&event_id));
let sockets_contains_key = if let Entry::Occupied(_) = self.sockets.entry(event_id) {
true
} else {
false
};

let peers_contains_key = if let Entry::Occupied(_) = self.sockets.entry(event_id) {
true
} else {
false
};

assert!(!sockets_contains_key);
assert!(!peers_contains_key);

self.sockets.insert(event_id, socket);
self.peers.insert(event_id, new_convo);
Expand Down Expand Up @@ -1903,7 +1916,7 @@ impl PeerNetwork {
};

// event ID already used?
if self.peers.contains_key(&event_id) {
if let Entry::Occupied(_) = self.peers.entry(event_id) {
warn!(
"Already have an event {}: {:?}",
event_id,
Expand Down Expand Up @@ -2042,7 +2055,7 @@ impl PeerNetwork {
/// Process any newly-connecting sockets
fn process_connecting_sockets(&mut self, poll_state: &mut NetworkPollState) {
for event_id in poll_state.ready.iter() {
if self.connecting.contains_key(event_id) {
if let Entry::Occupied(_) = self.connecting.entry(*event_id) {
let (socket, outbound, _) = self.connecting.remove(event_id).unwrap();
let sock_str = format!("{:?}", &socket);
if let Err(_e) = self.register_peer(*event_id, socket, outbound) {
Expand Down Expand Up @@ -3234,9 +3247,9 @@ impl PeerNetwork {

local_blocks.push(BlocksDatum(consensus_hash, block));

if !lowest_reward_cycle_with_missing_block.contains_key(nk) {
if let Entry::Vacant(_) = lowest_reward_cycle_with_missing_block.entry(nk) {
lowest_reward_cycle_with_missing_block
.insert(nk.clone(), reward_cycle);
.insert(nk, reward_cycle);
}

total_blocks_to_broadcast += 1;
Expand Down Expand Up @@ -3295,9 +3308,9 @@ impl PeerNetwork {

local_microblocks.push((index_block_hash, microblocks));

if !lowest_reward_cycle_with_missing_block.contains_key(nk) {
if let Entry::Vacant(_) = lowest_reward_cycle_with_missing_block.entry(nk) {
lowest_reward_cycle_with_missing_block
.insert(nk.clone(), reward_cycle);
.insert(nk, reward_cycle);
}

total_microblocks_to_broadcast += 1;
Expand Down
19 changes: 16 additions & 3 deletions stackslib/src/net/poll.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
// You should have received a copy of the GNU General Public License
// along with this program. If not, see <http://www.gnu.org/licenses/>.

use std::collections::hash_map::Entry;
use std::collections::{HashMap, HashSet};
use std::io::{Error as io_error, ErrorKind, Read, Write};
use std::net::{Shutdown, SocketAddr};
Expand Down Expand Up @@ -155,8 +156,14 @@ impl NetworkState {
server_event: mio::Token(next_server_event),
};

let event_map_contains_key = if let Entry::Occupied(_) = self.event_map.entry(next_server_event) {
true
} else {
false
};

assert!(
!self.event_map.contains_key(&next_server_event),
!event_map_contains_key,
"BUG: failed to generate an unused server event ID"
);

Expand Down Expand Up @@ -192,7 +199,7 @@ impl NetworkState {
}

// if the event ID is in use, then find another one
let event_id = if self.event_map.contains_key(&hint_event_id) {
let event_id = if let Entry::Occupied(_) = self.event_map.entry(hint_event_id) {
self.next_event_id()?
} else {
hint_event_id
Expand Down Expand Up @@ -236,8 +243,14 @@ impl NetworkState {
event_id: usize,
sock: &mio_net::TcpStream,
) -> Result<(), net_error> {
let event_map_contains_key = if let Entry::Occupied(_) = self.event_map.entry(event_id) {
true
} else {
false
};

assert!(
self.event_map.contains_key(&event_id),
event_map_contains_key,
"BUG: no such socket {}",
event_id
);
Expand Down
Loading