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.

Sign up for GitHub

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

Remove unneeded clone #94

Closed
wants to merge 3 commits into from
Closed

Remove unneeded clone #94

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
82a3d86
avoid not needed clone when calling join_subscriber
vincentfretin Aug 2, 2021
0dbda29
use Vec::with_capacity to create senders Vec
vincentfretin Aug 2, 2021
22926cf
no need to clone messages_tx here
vincentfretin Aug 2, 2021
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
6 changes: 3 additions & 3 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,6 @@ extern "C" fn init(callbacks: *mut PluginCallbacks, config_path: *const c_char)
match unsafe { callbacks.as_ref() } {
Some(c) => {
unsafe { CALLBACKS = Some(c) };
let mut senders = Vec::new();
let num_threads = if message_threads == 0 {
let cpus = num_cpus::get();
janus_info!("message_threads is set to 0, setting it to {} (number of cpus)", cpus);
Expand All @@ -267,9 +266,10 @@ extern "C" fn init(callbacks: *mut PluginCallbacks, config_path: *const c_char)
message_threads
};

let mut senders = Vec::with_capacity(num_threads);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating Vec with Vec::with_capacity is a best practice when you know the size in advance.
From memory allocation point of view, I read there is no allocation when using Vec::new, there is an allocation for 4 items on the first push, then a new allocation with capacity x2 when the capacity is reached and so on.
Here nothing changes fundamentally, this code is executed when the plugin initialize, we may save an extra allocation if you have more than 4 cpus. :)

for i in 0..num_threads {
let (messages_tx, messages_rx) = mpsc::sync_channel(0);
senders.push(messages_tx.clone());
senders.push(messages_tx);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

messages_tx is added to senders Vec, then the Vec is set on the static variable.
There is no need to clone messages_tx, this is not moved to the created thread. Only messages_rx is moved to the created thread.


thread::Builder::new()
.name(format!("sfu msg {}", i))
Expand Down Expand Up @@ -479,7 +479,7 @@ fn process_join(from: &Arc<Session>, room_id: RoomId, user_id: UserId, subscribe
switchboard.join_publisher(Arc::clone(from), user_id.clone(), room_id.clone());
notify_except(&notification, &user_id, switchboard.publishers_occupying(&room_id));
} else {
switchboard.join_subscriber(Arc::clone(from), user_id.clone(), room_id.clone());
switchboard.join_subscriber(Arc::clone(from), user_id, room_id);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If found this one with cargo clippy.
user_id and room_id are owned String from the MessageKind::Join message (serde DeserializeOwned) that are already moved to process_join. Those two String are not used after this, so no need to clone, join_subscriber can own them.

Note that in process_block, the whom String is not cloned, the String is moved to switchboard.establish_block without issue.

}

if let Some(subscription) = subscribe {
Expand Down