Skip to content

Conversation

@kongxiangyan
Copy link

@kongxiangyan kongxiangyan commented Dec 8, 2025

After reviewing the source code, I noticed that some parts of the logic do not appear to have any observable effect:

  • The Recursed flag seems to be unnecessary.
  • The cycle field (Link.version) also appears to be redundant.
  • The isValidLink check inside propagate does not seem to be required.

After removing these parts, all existing tests still pass successfully, and the overall performance appears to improve.

However, I am not fully certain whether removing these pieces is entirely safe in all scenarios. I would appreciate guidance from the maintainers or reviewers on whether this simplification is acceptable.

This PR proposes removing the above logic to simplify the implementation and improve performance.

Copilot AI review requested due to automatic review settings December 8, 2025 08:57
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the reactive system by removing version-based link tracking and the Recursed flag, simplifying the change propagation logic.

  • Removes the version field from Link interface and cycle global variable used for version tracking
  • Removes the ReactiveFlags.Recursed flag (value 8) from the reactive flags enum
  • Simplifies the propagation logic by removing the isValidLink() function and associated conditional branches

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/system.ts Removes version field from Link interface, removes Recursed flag enum value, updates link() function signature to remove version parameter, simplifies propagation logic by removing isValidLink() function and version/Recursed checks
src/index.ts Removes cycle global variable, updates all link() function calls to remove version/cycle parameter from effect(), effectScope(), computedOper(), and signalOper() functions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@kongxiangyan
Copy link
Author

bench before remove:

$ pnpm run bench

> [email protected] bench D:\Sources\forked\alien-signals
> npm run build && node --jitless --expose-gc benchs/propagate.mjs


> [email protected] build
> node ./build.js

Warning: disabling flag --expose_wasm due to conflicting flags
clk: ~0.07 GHz
cpu: AMD Ryzen 9 7945HX with Radeon Graphics        
runtime: node 20.17.0 (x64-win32)

| benchmark            |              avg |         min |         p75 |         p99 |         max |
| -------------------- | ---------------- | ----------- | ----------- | ----------- | ----------- |
| propagate: 1 * 1     | `847.44 ns/iter` | `825.98 ns` | `851.54 ns` | `892.48 ns` | `910.21 ns` |
| propagate: 1 * 10    | `  3.49 µs/iter` | `  3.43 µs` | `  3.51 µs` | `  3.55 µs` | `  3.55 µs` |
| propagate: 1 * 100   | ` 29.52 µs/iter` | ` 29.15 µs` | ` 29.65 µs` | ` 29.70 µs` | ` 29.97 µs` |
| propagate: 10 * 1    | `  6.53 µs/iter` | `  6.48 µs` | `  6.54 µs` | `  6.56 µs` | `  6.58 µs` |
| propagate: 10 * 10   | ` 33.29 µs/iter` | ` 32.94 µs` | ` 33.53 µs` | ` 33.57 µs` | ` 33.62 µs` |
| propagate: 10 * 100  | `289.67 µs/iter` | `277.90 µs` | `291.00 µs` | `319.50 µs` | `359.20 µs` |
| propagate: 100 * 1   | ` 62.79 µs/iter` | ` 61.28 µs` | ` 63.24 µs` | ` 63.42 µs` | ` 64.77 µs` |
| propagate: 100 * 10  | `325.02 µs/iter` | `314.40 µs` | `325.70 µs` | `359.90 µs` | `400.50 µs` |
| propagate: 100 * 100 | `  3.08 ms/iter` | `  2.98 ms` | `  3.09 ms` | `  3.72 ms` | `  4.16 ms` |

bench after remove:

$ pnpm run bench

> [email protected] bench D:\Sources\forked\alien-signals
> npm run build && node --jitless --expose-gc benchs/propagate.mjs


> [email protected] build
> node ./build.js

Warning: disabling flag --expose_wasm due to conflicting flags
clk: ~0.07 GHz
cpu: AMD Ryzen 9 7945HX with Radeon Graphics        
runtime: node 20.17.0 (x64-win32)

| benchmark            |              avg |         min |         p75 |         p99 |         max |
| -------------------- | ---------------- | ----------- | ----------- | ----------- | ----------- |
| propagate: 1 * 1     | `804.48 ns/iter` | `762.70 ns` | `819.51 ns` | `876.90 ns` | `894.04 ns` |
| propagate: 1 * 10    | `  3.32 µs/iter` | `  3.27 µs` | `  3.35 µs` | `  3.38 µs` | `  3.38 µs` |
| propagate: 1 * 100   | ` 28.08 µs/iter` | ` 27.60 µs` | ` 28.27 µs` | ` 28.45 µs` | ` 28.59 µs` |
| propagate: 10 * 1    | `  6.10 µs/iter` | `  5.92 µs` | `  6.18 µs` | `  6.26 µs` | `  6.28 µs` |
| propagate: 10 * 10   | ` 31.23 µs/iter` | ` 30.77 µs` | ` 31.42 µs` | ` 31.54 µs` | ` 31.68 µs` |
| propagate: 10 * 100  | `274.22 µs/iter` | `257.80 µs` | `274.60 µs` | `416.20 µs` | `636.90 µs` |
| propagate: 100 * 1   | ` 58.06 µs/iter` | ` 56.17 µs` | ` 58.35 µs` | ` 60.32 µs` | ` 63.45 µs` |
| propagate: 100 * 10  | `308.92 µs/iter` | `290.10 µs` | `310.50 µs` | `366.50 µs` | `560.60 µs` |
| propagate: 100 * 100 | `  2.90 ms/iter` | `  2.83 ms` | `  2.91 ms` | `  2.99 ms` | `  3.02 ms` |

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.

1 participant