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

Task Edges Set/List #8624

Merged
merged 45 commits into from
Jul 2, 2024
Merged

Task Edges Set/List #8624

merged 45 commits into from
Jul 2, 2024

Conversation

sokra
Copy link
Member

@sokra sokra commented Jun 28, 2024

Description

  • Task children and dependencies are stored together in a single datastructure -> task edges
  • task edges is a map keyed by target task id and using a bitfield for common edges (child, output, cell 0)
  • removed lazy_remove_children feature flag -> this is always on now (Disabling it doesn't make sense as it negatively affects performance, we had the feature flag in case it it is broken, but seems fine)
  • When a task becomes Done, we use a Vec instead of a HashMap for better memory efficiency. It doesn't need to modified after that.

Testing Instructions

@sokra sokra requested a review from a team as a code owner June 28, 2024 06:15
Copy link

vercel bot commented Jun 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rust-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 1, 2024 4:37pm
8 Ignored Deployments
Name Status Preview Comments Updated (UTC)
examples-basic-web ⬜️ Ignored (Inspect) Visit Preview Jul 1, 2024 4:37pm
examples-designsystem-docs ⬜️ Ignored (Inspect) Visit Preview Jul 1, 2024 4:37pm
examples-gatsby-web ⬜️ Ignored (Inspect) Visit Preview Jul 1, 2024 4:37pm
examples-kitchensink-blog ⬜️ Ignored (Inspect) Visit Preview Jul 1, 2024 4:37pm
examples-native-web ⬜️ Ignored (Inspect) Visit Preview Jul 1, 2024 4:37pm
examples-svelte-web ⬜️ Ignored (Inspect) Visit Preview Jul 1, 2024 4:37pm
examples-tailwind-web ⬜️ Ignored (Inspect) Visit Preview Jul 1, 2024 4:37pm
examples-vite-web ⬜️ Ignored (Inspect) Visit Preview Jul 1, 2024 4:37pm

Copy link
Contributor

github-actions bot commented Jun 28, 2024

🟢 Turbopack Benchmark CI successful 🟢

Thanks

@sokra sokra requested a review from bgw June 28, 2024 06:17
Copy link
Contributor

github-actions bot commented Jun 28, 2024

⚠️ This change may fail to build next-swc.

Logs

error: failed to select a version for `triomphe`.
    ... required by package `hstr v0.2.8`
    ... which satisfies dependency `hstr = "^0.2.8"` (locked to 0.2.8) of package `swc_atoms v0.6.7`
    ... which satisfies dependency `swc_atoms = "^0.6.5"` (locked to 0.6.7) of package `swc_core v0.95.4`
    ... which satisfies dependency `swc_core = "^0.95.4"` (locked to 0.95.4) of package `wasm v0.0.0 (/root/actions-runner/_work/turbo/turbo/packages/next-swc/crates/wasm)`
versions that meet the requirements `^0.1.11` (locked to 0.1.11) are: 0.1.11

all possible versions conflict with previously selected packages.

  previously selected package `triomphe v0.1.13`
    ... which satisfies dependency `triomphe = "^0.1.13"` of package `turbo-tasks v0.1.0 (https://github.com/vercel/turbo?rev=413cfbf596e054d65edb6870658103338e856cab#78c44bd1)`
    ... which satisfies git dependency `turbo-tasks` (locked to 0.1.0) of package `next-build-test v0.1.0 (/root/actions-runner/_work/turbo/turbo/packages/next-swc/crates/next-build-test)`

failed to select a version for `triomphe` which could resolve this conflict

See job summary for details

Copy link
Contributor

github-actions bot commented Jun 28, 2024

⚠️ CI failed ⚠️

The following steps have failed in CI:

  • Turbopack Rust tests (mac/win, non-blocking)

See workflow summary for details

Copy link
Member

@bgw bgw left a comment

Choose a reason for hiding this comment

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

I read through the edges set implementation, but haven't gotten to reviewing how it's used yet.

crates/turbo-tasks-auto-hash-map/src/map.rs Show resolved Hide resolved
crates/turbo-tasks-auto-hash-map/src/map.rs Outdated Show resolved Hide resolved
crates/turbo-tasks-memory/src/edges_set.rs Show resolved Hide resolved
crates/turbo-tasks-memory/src/edges_set.rs Outdated Show resolved Hide resolved
crates/turbo-tasks-memory/src/edges_set.rs Outdated Show resolved Hide resolved
crates/turbo-tasks-memory/src/edges_set.rs Show resolved Hide resolved
crates/turbo-tasks-memory/src/edges_set.rs Show resolved Hide resolved
crates/turbo-tasks-memory/src/edges_set.rs Outdated Show resolved Hide resolved
crates/turbo-tasks-memory/src/edges_set.rs Outdated Show resolved Hide resolved
Comment on lines 554 to 538
pub fn into_set(self) -> TaskEdgesSet {
TaskEdgesSet {
edges: Box::new(self.edges.into_vec().into_iter().collect()),
}
}
Copy link
Member

Choose a reason for hiding this comment

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

How common is this operation? I bet if we were clever we could do an optimization for the case where we're converting into a SmallVec that avoids the iterator.

crates/turbo-tasks-auto-hash-map/src/map.rs Outdated Show resolved Hide resolved
crates/turbo-tasks-memory/src/task.rs Outdated Show resolved Hide resolved
@sokra sokra merged commit 52ed46b into main Jul 2, 2024
57 of 60 checks passed
@sokra sokra deleted the sokra/edges-set-list branch July 2, 2024 06:39
@bgw
Copy link
Member

bgw commented Jul 2, 2024

Thanks for making the suggested changes! The edge_set implementation LGTM!

bgw added a commit to vercel/next.js that referenced this pull request Jul 3, 2024
Tobias Koppers - fix typo (vercel/turborepo#8619)
Benjamin Woodruff - Store aggregate read/execute count statistics
(vercel/turborepo#8286)
Tobias Koppers - box InProgress task state (vercel/turborepo#8644)
Tobias Koppers - Task Edges Set/List (vercel/turborepo#8624)
Benjamin Woodruff - Memory: Use `triomphe::Arc` for `SharedReference`
(vercel/turborepo#8622)
Will Binns-Smith - chore: release npm packages (vercel/turborepo#8614)
Will Binns-Smith - devlow-bench: add git branch and sha to datapoints
(vercel/turborepo#8602)

---

Fixes a `triomphe` package version conflict between turbopack and swc by
bumping it from 0.1.11 to 0.1.13.
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 25, 2024
### Description

* Task children and dependencies are stored together in a single
datastructure -> task edges
* task edges is a map keyed by target task id and using a bitfield for
common edges (child, output, cell 0)
* removed lazy_remove_children feature flag -> this is always on now
(Disabling it doesn't make sense as it negatively affects performance,
we had the feature flag in case it it is broken, but seems fine)
* When a task becomes Done, we use a Vec instead of a HashMap for better
memory efficiency. It doesn't need to modified after that.

### Testing Instructions

<!--
  Give a quick description of steps to test your changes.
-->

---------

Co-authored-by: Benjamin Woodruff <benjamin.woodruff@vercel.com>
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
### Description

* Task children and dependencies are stored together in a single
datastructure -> task edges
* task edges is a map keyed by target task id and using a bitfield for
common edges (child, output, cell 0)
* removed lazy_remove_children feature flag -> this is always on now
(Disabling it doesn't make sense as it negatively affects performance,
we had the feature flag in case it it is broken, but seems fine)
* When a task becomes Done, we use a Vec instead of a HashMap for better
memory efficiency. It doesn't need to modified after that.

### Testing Instructions

<!--
  Give a quick description of steps to test your changes.
-->

---------

Co-authored-by: Benjamin Woodruff <benjamin.woodruff@vercel.com>
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
### Description

* Task children and dependencies are stored together in a single
datastructure -> task edges
* task edges is a map keyed by target task id and using a bitfield for
common edges (child, output, cell 0)
* removed lazy_remove_children feature flag -> this is always on now
(Disabling it doesn't make sense as it negatively affects performance,
we had the feature flag in case it it is broken, but seems fine)
* When a task becomes Done, we use a Vec instead of a HashMap for better
memory efficiency. It doesn't need to modified after that.

### Testing Instructions

<!--
  Give a quick description of steps to test your changes.
-->

---------

Co-authored-by: Benjamin Woodruff <benjamin.woodruff@vercel.com>
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Aug 1, 2024
### Description

* Task children and dependencies are stored together in a single
datastructure -> task edges
* task edges is a map keyed by target task id and using a bitfield for
common edges (child, output, cell 0)
* removed lazy_remove_children feature flag -> this is always on now
(Disabling it doesn't make sense as it negatively affects performance,
we had the feature flag in case it it is broken, but seems fine)
* When a task becomes Done, we use a Vec instead of a HashMap for better
memory efficiency. It doesn't need to modified after that.

### Testing Instructions

<!--
  Give a quick description of steps to test your changes.
-->

---------

Co-authored-by: Benjamin Woodruff <benjamin.woodruff@vercel.com>
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Aug 16, 2024
Tobias Koppers - fix typo (vercel/turborepo#8619)
Benjamin Woodruff - Store aggregate read/execute count statistics
(vercel/turborepo#8286)
Tobias Koppers - box InProgress task state (vercel/turborepo#8644)
Tobias Koppers - Task Edges Set/List (vercel/turborepo#8624)
Benjamin Woodruff - Memory: Use `triomphe::Arc` for `SharedReference`
(vercel/turborepo#8622)
Will Binns-Smith - chore: release npm packages (vercel/turborepo#8614)
Will Binns-Smith - devlow-bench: add git branch and sha to datapoints
(vercel/turborepo#8602)

---

Fixes a `triomphe` package version conflict between turbopack and swc by
bumping it from 0.1.11 to 0.1.13.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants