Skip to content

perf(core): detect existing signal dependency without checking all pr…#67506

Draft
JoostK wants to merge 1 commit into
angular:mainfrom
JoostK:signals/link-insertion-perf
Draft

perf(core): detect existing signal dependency without checking all pr…#67506
JoostK wants to merge 1 commit into
angular:mainfrom
JoostK:signals/link-insertion-perf

Conversation

@JoostK

@JoostK JoostK commented Mar 7, 2026

Copy link
Copy Markdown
Member

…oducer links

This commit addresses a scaling issue in the signal dependency graph where the detection of duplicate dependency links would perform a linear scan across all consumer links of all producers. The linear scan is replaced with a version comparison of the dependency edge against the version of the node; if they are equal the existing dependency edge is valid.

Closes #67454

@JoostK JoostK added the target: patch This PR is targeted for the next patch release label Mar 7, 2026
@pullapprove pullapprove Bot requested review from AndrewKushnir and e-cline March 7, 2026 15:37
@angular-robot angular-robot Bot added area: performance Issues related to performance area: core Issues related to the framework runtime requires: TGP This PR requires a passing TGP before merging is allowed labels Mar 7, 2026
@ngbot ngbot Bot added this to the Backlog milestone Mar 7, 2026
@AndrewKushnir AndrewKushnir requested review from alxhub and pkozlowski-opensource and removed request for AndrewKushnir March 7, 2026 16:06
@JoostK JoostK added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 7, 2026
@JoostK JoostK marked this pull request as draft March 7, 2026 17:13
@JoostK JoostK force-pushed the signals/link-insertion-perf branch from d1029a1 to ac1c95b Compare March 7, 2026 22:05
@JoostK JoostK marked this pull request as ready for review March 7, 2026 22:06
@JoostK JoostK removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 7, 2026
@JoostK JoostK force-pushed the signals/link-insertion-perf branch from ac1c95b to bb41616 Compare March 7, 2026 22:47
thePunderWoman
thePunderWoman approved these changes Mar 17, 2026
View reviewed changes

@thePunderWoman thePunderWoman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

mturco
mturco approved these changes Mar 24, 2026
View reviewed changes

@mturco mturco left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed-for: primitives-shared

@thePunderWoman thePunderWoman removed the request for review from e-cline March 24, 2026 20:55
@thePunderWoman thePunderWoman added the action: merge The PR is ready for merge by the caretaker label Mar 24, 2026
@ngbot

ngbot Bot commented Mar 24, 2026

Copy link
Copy Markdown

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "google-internal-tests" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@JeanMeche JeanMeche removed the action: merge The PR is ready for merge by the caretaker label Mar 25, 2026
@JeanMeche

JeanMeche commented Mar 25, 2026

Copy link
Copy Markdown
Member

We're seeing at least 2 failing targets on the TGP but they don't seem very narrow.

One of the failing tests reports a NG0201 when evaluating a signal here: https://github.com/angular/components/blob/ff7d697ec5ed280d3e28029571bc86d7c0c2c3a8/src/material/tabs/tab-nav-bar/tab-nav-bar.ts#L262

@autonomobil

autonomobil commented Mar 27, 2026

Copy link
Copy Markdown

any update here? :) We're kinda stuck upgrading from v19 to v21 because of this issue

@JoostK

JoostK commented Mar 27, 2026

Copy link
Copy Markdown
Member Author

I haven't had a chance to look into any issues this may have caused, as indicated by the TGP results.

@autonomobil

autonomobil commented Apr 11, 2026

Copy link
Copy Markdown

I can't access the logs to investigate. Maybe just flaky and a restart helps?

@JoostK JoostK force-pushed the signals/link-insertion-perf branch from bb41616 to 0c6771d Compare April 12, 2026 19:28
@angular-robot angular-robot Bot requested a review from mturco April 12, 2026 19:28
@JoostK

JoostK commented Apr 12, 2026

Copy link
Copy Markdown
Member Author

I can't access the logs to investigate. Maybe just flaky and a restart helps?

It's not a flake; I've pushed a failing test that showcases the problem, will be looking into ways to address it.

@JoostK JoostK marked this pull request as draft April 12, 2026 19:31
@JoostK JoostK force-pushed the signals/link-insertion-perf branch 2 times, most recently from c8370db to fd9eb88 Compare April 15, 2026 21:12
@autonomobil

autonomobil commented May 11, 2026

Copy link
Copy Markdown

any update? :)

@JoostK JoostK force-pushed the signals/link-insertion-perf branch from fd9eb88 to cdc1f36 Compare May 11, 2026 17:59
@JoostK

JoostK commented May 12, 2026

Copy link
Copy Markdown
Member Author

any update? :)

It continues to be broken, apparently in different ways than what was recently accounted for. I do not know how, though.

@autonomobil

autonomobil commented Jun 9, 2026

Copy link
Copy Markdown

any update?

@JoostK

JoostK commented Jun 9, 2026

Copy link
Copy Markdown
Member Author

any update?

It continues to be broken, apparently in different ways than what was recently accounted for. I do not know how, though.

@JeanMeche

JeanMeche commented Jun 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Can you rebase your PR ? I'll try to spent some time investigating this a bit more.

@JeanMeche JeanMeche force-pushed the signals/link-insertion-perf branch from cdc1f36 to c603460 Compare June 9, 2026 23:02
@JoostK @alan-agius4
perf(core): detect existing signal dependency without checking all pr…
b7be621 
…oducer links

This commit addresses a scaling issue in the signal dependency graph where
the detection of duplicate dependency links would perform a linear scan across
all consumer links of all producers. The linear scan is replaced with a version
comparison of the dependency edge against the current epoch; if they are
equal the existing dependency edge is known to be valid in this epoch. This means
that the link won't be eligible for removal and therefore doesn't have to be
recreated.

Closes angular#67454
@alan-agius4 alan-agius4 force-pushed the signals/link-insertion-perf branch from c603460 to b7be621 Compare June 10, 2026 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thePunderWoman thePunderWoman thePunderWoman approved these changes

@mturco mturco Awaiting requested review from mturco

+1 more reviewer

@milomg milomg milomg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area: core Issues related to the framework runtime area: performance Issues related to performance requires: TGP This PR requires a passing TGP before merging is allowed target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

PR #62284 (commit 3c008c9) introduced an O(N²) performance regression for computed signals that depend on many producers.

6 participants

@JoostK @JeanMeche @autonomobil @mturco @thePunderWoman @milomg