js-framework-benchmark icon indicating copy to clipboard operation
js-framework-benchmark copied to clipboard

Update Angular to V18

Open krausest opened this issue 1 year ago • 11 comments

Would be great if we moved angular to v18. I tried to update with ng update, but results didn't look good. Would be great if @pkozlowski-opensource or @birkskyum could take a look. Any chance?

krausest avatar Jun 05 '24 19:06 krausest

I guess it could be interesting to try the new nozone with the signals one as well

birkskyum avatar Jun 05 '24 19:06 birkskyum

I tried to update with ng update, but results didn't look good.

@krausest did the update fail? Or the perf results weren't good? Happy to help, just trying to understand what is going on.

I guess it could be interesting to try the new nozone with the signals one as well

nozone for sure - and those are not tied to signals (I mean, you can run zoneless without using signals).

pkozlowski-opensource avatar Jun 05 '24 20:06 pkozlowski-opensource

The update itself was fine. For angular-cf-signals I bootstrapped with provideExperimentalZonelessChangeDetection() and added ChangeDetectionStrategy.OnPush to the component. After that select rows took 87 msecs instead of 6.5 msecs. Swap rows was similarly bad.

krausest avatar Jun 05 '24 20:06 krausest

If it helps I set up a PR with my changes here: https://github.com/krausest/js-framework-benchmark/pull/1695

krausest avatar Jun 05 '24 20:06 krausest

Those were the results I got. I can remove that image again if you want me to, since I really hope I made a silly mistake (left new keyed, right old keyed results): Screenshot 2024-06-05 at 21 46 22

krausest avatar Jun 05 '24 20:06 krausest

Those were the results I got. I can remove that image again if you want me to, since I really hope I made a silly mistake (left new keyed, right old keyed results):

@krausest thnx for following up on this one. I will have a deeper but I don't see any "silly mistake" looking at the PR. What I do expect that the change detection scheduler we are using might not be playing nicely with the benchmark / way we measure. But yeh, this is just a suspicion for now, will profile!

pkozlowski-opensource avatar Jun 06 '24 06:06 pkozlowski-opensource

@krausest actually I think I know what is going on - at least partly.

For angular-cf-signals I bootstrapped with provideExperimentalZonelessChangeDetection() and added ChangeDetectionStrategy.OnPush to the component. After that select rows took 87 msecs instead of 6.5 msecs. Swap rows was similarly bad.

Looking at the change in https://github.com/krausest/js-framework-benchmark/pull/1695/files#diff-ede0fde54eaba73652ad146e9d60f72e603ed149428291a7a7fc83c3d667c26aR7 it would make running the change detection at least twice. The reason for this is that we've got now zoneless scheduling change detection and manual calls to detect changes.

pkozlowski-opensource avatar Jun 06 '24 06:06 pkozlowski-opensource

I've removed double calls in a separate commit (see https://github.com/pkozlowski-opensource/js-framework-benchmark/commit/307580b3c4bcd1cd83aef10bef3f075f3cd7f20f) and it significantly improves numbers but still performs worse as compared to the zone-based version.

The interesting thing is that under a profiler I can see select rows performing great so, as noted above, I think that our zoneless scheduler interact in an undesirable way with the benchmarking infrastructure. I didn't get to the bottom of it yet.

So I think we've got 2 issues:

  • double calls to change detection (removed in https://github.com/pkozlowski-opensource/js-framework-benchmark/commit/307580b3c4bcd1cd83aef10bef3f075f3cd7f20f)
  • new zoneless scheduler and benchmarking

Given this I would suggest the following:

  • revert switch to the experimental zoneless (left comments in a PR but can also prepare a patch commit) so we can compare evolution from one version to another;
  • work on adding experimental zoneless to be benchmark in a separate PR

WDYT?

pkozlowski-opensource avatar Jun 06 '24 14:06 pkozlowski-opensource

May I ask which is the latest modified PR? I would also like to participate and take a look

wszgrcy avatar Jun 10 '24 01:06 wszgrcy

#1696 contains one new zoneless implementation. The existing angular implementations were bumped to v18 in https://github.com/krausest/js-framework-benchmark/commit/40fc82277103e622c1b276d8c641101c033084e8 (without switching to new zoneless).

krausest avatar Jun 10 '24 04:06 krausest

Those were the results I got. I can remove that image again if you want me to, since I really hope I made a silly mistake (left new keyed, right old keyed results): Screenshot 2024-06-05 at 21 46 22

If the test results of different devices have consistent standards Perhaps this is a performance issue. Need Angular official to resolve it? Because I feel like there's nothing more to optimize image

wszgrcy avatar Jun 11 '24 00:06 wszgrcy

Should this be updated with the released angular v19?

NateRadebaugh avatar Dec 06 '24 02:12 NateRadebaugh

Should this be updated with the released angular v19?

Yes, done in #1786.

krausest avatar Dec 11 '24 05:12 krausest