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

reduce number of allocations #2833

Merged
merged 8 commits into from
Dec 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment to explain that VecDeque allocates 7 elements by default. Otherwise I expect this to be accidentally changed back in the future.

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