Skip to content

Commit

Permalink
reduce number of allocations (#2833)
Browse files Browse the repository at this point in the history
VecDeque::new always allocates 7 elements
  • Loading branch information
sokra committed Dec 5, 2022
1 parent 4f247be commit dc36fc4
Show file tree
Hide file tree
Showing 8 changed files with 26 additions and 15 deletions.
3 changes: 3 additions & 0 deletions clippy.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,7 @@ disallowed-methods = [
# sets), but the hash **is not stable** and must not be observed.
# Use Xxh3Hash64Hasher::write with value's bytes directly.
"std::hash::Hasher::hash",
# We forbid the use of VecDeque::new as it allocates, which is kind of unexpected
# Instead use VecDeque::with_capacity to make it explicit or opt-out of that.
"std::collections::VecDeque::new",
]
22 changes: 15 additions & 7 deletions crates/turbo-tasks-memory/src/task.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::{
borrow::Cow,
cell::RefCell,
cmp::Ordering,
cmp::{max, Ordering},
collections::VecDeque,
fmt::{self, Debug, Display, Formatter, Write},
future::Future,
Expand Down Expand Up @@ -743,6 +743,9 @@ impl Task {
}
}

if queue.capacity() == 0 {
queue.reserve(max(children.len(), SPLIT_OFF_QUEUE_AT * 2));
}
queue.extend(children.iter().copied().map(|child| (child, depth + 1)));

// add to dirty list of the scope (potentially schedule)
Expand All @@ -764,7 +767,8 @@ impl Task {
backend: &MemoryBackend,
turbo_tasks: &dyn TurboTasksBackendApi,
) {
let mut queue = VecDeque::new();
// VecDeque::new() would allocate with 7 items capacity. We don't want that.
let mut queue = VecDeque::with_capacity(0);
self.add_to_scope_internal_shallow(
id,
is_optimization_scope,
Expand Down Expand Up @@ -908,6 +912,9 @@ impl Task {
TaskScopes::Inner(ref mut set, _) => {
if set.remove(id) {
self.remove_self_from_scope(&mut state, id, backend, turbo_tasks);
if queue.capacity() == 0 {
queue.reserve(max(state.children.len(), SPLIT_OFF_QUEUE_AT * 2));
}
queue.extend(state.children.iter().copied());
drop(state);
}
Expand All @@ -921,7 +928,8 @@ impl Task {
backend: &MemoryBackend,
turbo_tasks: &dyn TurboTasksBackendApi,
) {
let mut queue = VecDeque::new();
// VecDeque::new() would allocate with 7 items capacity. We don't want that.
let mut queue = VecDeque::with_capacity(0);
self.remove_from_scope_internal_shallow(id, backend, turbo_tasks, &mut queue);
run_remove_from_scope_queue(queue, id, backend, turbo_tasks);
}
Expand Down Expand Up @@ -1540,8 +1548,8 @@ pub fn run_add_to_scope_queue(
&mut queue,
);
});
if queue.len() > SPLIT_OFF_QUEUE_AT {
let split_off_queue = queue.split_off(SPLIT_OFF_QUEUE_AT);
while queue.len() > SPLIT_OFF_QUEUE_AT {
let split_off_queue = queue.split_off(queue.len() - SPLIT_OFF_QUEUE_AT);
turbo_tasks.schedule_backend_foreground_job(backend.create_backend_job(
Job::AddToScopeQueue(split_off_queue, id, is_optimization_scope),
));
Expand All @@ -1560,8 +1568,8 @@ pub fn run_remove_from_scope_queue(
backend.with_task(child, |child| {
child.remove_from_scope_internal_shallow(id, backend, turbo_tasks, &mut queue);
});
if queue.len() > SPLIT_OFF_QUEUE_AT {
let split_off_queue = queue.split_off(SPLIT_OFF_QUEUE_AT);
while queue.len() > SPLIT_OFF_QUEUE_AT {
let split_off_queue = queue.split_off(queue.len() - SPLIT_OFF_QUEUE_AT);

turbo_tasks.schedule_backend_foreground_job(
backend.create_backend_job(Job::RemoveFromScopeQueue(split_off_queue, id)),
Expand Down
2 changes: 1 addition & 1 deletion crates/turbopack-core/src/chunk/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ async fn chunk_content_internal<I: FromChunkableAsset>(
let mut chunks = Vec::new();
let mut async_chunk_groups = Vec::new();
let mut external_asset_references = Vec::new();
let mut queue = VecDeque::new();
let mut queue = VecDeque::with_capacity(32);

let chunk_item = I::from_asset(context, entry).await?.unwrap();
queue.push_back(ChunkContentWorkItem::AssetReferences(
Expand Down
4 changes: 2 additions & 2 deletions crates/turbopack-core/src/reference/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ impl SingleAssetReferenceVc {
pub async fn all_referenced_assets(asset: AssetVc) -> Result<AssetsVc> {
let references_set = asset.references().await?;
let mut assets = Vec::new();
let mut queue = VecDeque::new();
let mut queue = VecDeque::with_capacity(32);
for reference in references_set.iter() {
queue.push_back(reference.resolve_reference());
}
Expand Down Expand Up @@ -137,7 +137,7 @@ pub async fn all_referenced_assets(asset: AssetVc) -> Result<AssetsVc> {
#[turbo_tasks::function]
pub async fn all_assets(asset: AssetVc) -> Result<AssetsVc> {
// TODO need to track import path here
let mut queue = VecDeque::new();
let mut queue = VecDeque::with_capacity(32);
queue.push_back((asset, all_referenced_assets(asset)));
let mut assets = HashSet::new();
assets.insert(asset);
Expand Down
4 changes: 2 additions & 2 deletions crates/turbopack-create-test-app/src/test_app_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ const SETUP_EFFECT_PROPS: &str = indoc! {r#"
let EFFECT_PROPS = {};
"#};
const SETUP_EVAL: &str = indoc! {r#"
/* @turbopack-bench:eval-start */
/* @turbopack-bench:eval-start */
/* @turbopack-bench:eval-end */
"#};
const USE_EFFECT: &str = indoc! {r#"
Expand Down Expand Up @@ -115,7 +115,7 @@ impl TestAppBuilder {
let mut remaining_directories = self.directories_count;
let mut remaining_dynamic_imports = self.dynamic_import_count;

let mut queue = VecDeque::new();
let mut queue = VecDeque::with_capacity(32);
queue.push_back((src.join("triangle.jsx"), 0));
remaining_modules -= 1;
let mut is_root = true;
Expand Down
2 changes: 1 addition & 1 deletion crates/turbopack-dev-server/src/source/asset_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ impl AssetGraphContentSourceVc {
let mut map = HashMap::new();
let root_path = this.root_path.await?;
let mut assets = Vec::new();
let mut queue = VecDeque::new();
let mut queue = VecDeque::with_capacity(32);
let mut assets_set = HashSet::new();
let root_assets = this.root_assets.await?;
if let Some(state) = &this.state {
Expand Down
2 changes: 1 addition & 1 deletion crates/turbopack-tests/tests/snapshot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ async fn run_test(resource: String) -> Result<FileSystemPathVc> {
.await?;

let mut seen = HashSet::new();
let mut queue = VecDeque::new();
let mut queue = VecDeque::with_capacity(32);
for chunk in chunks {
queue.push_back(chunk.as_asset());
}
Expand Down
2 changes: 1 addition & 1 deletion crates/turbopack/tests/helpers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pub fn print_changeset(changeset: &Changeset) -> String {
assert!(changeset.split == "\n");
let mut result = String::from("--- DIFF ---\n- Expected\n+ Actual\n------------\n");
const CONTEXT_LINES: usize = 3;
let mut context = VecDeque::new();
let mut context = VecDeque::with_capacity(CONTEXT_LINES);
let mut need_context = CONTEXT_LINES;
let mut has_spacing = false;
for diff in changeset.diffs.iter() {
Expand Down

0 comments on commit dc36fc4

Please sign in to comment.