Skip to content

chore: remove perf regression for signals re-enablement#5347

Merged
jhefferman-sfdc merged 1 commit intomasterfrom
jhefferman/remove-perf-regression-checks
Apr 28, 2025
Merged

chore: remove perf regression for signals re-enablement#5347
jhefferman-sfdc merged 1 commit intomasterfrom
jhefferman/remove-perf-regression-checks

Conversation

@jhefferman-sfdc
Copy link
Contributor

@jhefferman-sfdc jhefferman-sfdc commented Apr 28, 2025

Remove perf regression for signals re-enablement. Once removed, we can re-enable ENABLE_EXPERIMENTAL_SIGNALS in main

Details

Does this pull request introduce a breaking change?

  • 😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • 🤞 No, it does not introduce an observable change.

GUS work item

W-18381293

@jhefferman-sfdc jhefferman-sfdc requested a review from a team as a code owner April 28, 2025 15:48
@jhefferman-sfdc jhefferman-sfdc changed the title chore: remove checks that can cause perf regression for signals re-en… chore: remove perf regression for signals re-enablement Apr 28, 2025
Comment on lines -45 to -47
safeHasProp(target, 'value') &&
safeHasProp(target, 'subscribe') &&
isFunction(target.subscribe) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we no longer checking the shape target to make sure it's a signal? Won't that potentially break things?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assumed this was because checking for the properties doesn't guarantee correctness and it's up to the consumer to use the API properly? CC: @divmain

Copy link
Contributor

Choose a reason for hiding this comment

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

isTrustedSignal(target) is adequate. Objects won't be in the WeakMap unless they're actual signals, which means they'll have the required shape.

If we remove the trusted signal mechanism in the future, we'll have to revisit how to detect signals properly without risking performance regressions. For the specific regression that we encountered, we could add !Array.isArray(target) as a condition. In an case, we can deal with this when/if we remove the trusted signals mechanism. That'll be a couple of years out at the earliest.

Copy link
Contributor

Choose a reason for hiding this comment

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

@divmain turns out this is not adequate if the trusted signal tracking set isn't initialized. It defaults to assuming everything is a trusted signal, and therefore tries to subscribe to every mutated object. It doesn't break the framework because it's wrapped in a try/catch, but it's a major console noise and a perf hit. Oops! 😓

#5492 (comment)

Comment on lines -45 to -47
safeHasProp(target, 'value') &&
safeHasProp(target, 'subscribe') &&
isFunction(target.subscribe) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

isTrustedSignal(target) is adequate. Objects won't be in the WeakMap unless they're actual signals, which means they'll have the required shape.

If we remove the trusted signal mechanism in the future, we'll have to revisit how to detect signals properly without risking performance regressions. For the specific regression that we encountered, we could add !Array.isArray(target) as a condition. In an case, we can deal with this when/if we remove the trusted signals mechanism. That'll be a couple of years out at the earliest.

@jhefferman-sfdc jhefferman-sfdc merged commit 852f9c1 into master Apr 28, 2025
6 checks passed
@jhefferman-sfdc jhefferman-sfdc deleted the jhefferman/remove-perf-regression-checks branch April 28, 2025 19:02
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