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

Potential @vue/reactivity bug discovered during benchmark stress tests #11928

Closed
transitive-bullshit opened this issue Sep 15, 2024 · 2 comments · Fixed by #11944
Closed

Potential @vue/reactivity bug discovered during benchmark stress tests #11928

transitive-bullshit opened this issue Sep 15, 2024 · 2 comments · Fixed by #11944
Labels
❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. regression scope: reactivity

Comments

@transitive-bullshit
Copy link

transitive-bullshit commented Sep 15, 2024

Vue version

@vue/reactivity latest v3.5.5

Note that this issue does not use any other Vue packages but rather only relies on @vue/reactivity directly.

Link to minimal reproduction

https://github.com/transitive-bullshit/js-reactivity-benchmark/tree/feature/minimal-issues-repro-vue-reactivity

Steps to reproduce

  1. Checkout https://github.com/transitive-bullshit/js-reactivity-benchmark and switch to the minimal repro branch feature/minimal-issues-repro-vue-reactivity branch
  2. Run pnpm install
  3. Run pnpm test to make sure everything's working locally
  4. Run pnpm bench to reproduce the issue

When you run the benchmark, the @vue/reactivity adapter will hang on this line.

All other benchmark tests work fine, so I've removed them from this branch. The benchmark stress test is pretty simple. It is adapted from the unit tests in cellx. It creates a series of N layers, each containing 4 computed properties depending on the previous layer's signal values. It then performs a read from the last layer and makes sure that the value matches the expected value.

@vue/reactivity is the only framework to fail this particular test, and what makes me even more sure that this may be a bug in Vue is that if I remove these 4 effect lines, then the benchmark tests seem to run fine.

What is expected?

The benchmark test should run fine, with each one completing in a few seconds at most.

What is actually happening?

The benchmark test for @vue/reactivity hangs as described above.

CleanShot 2024-09-15 at 03 26 17@2x

System Info

- @vue/reactivity latest v3.5.5
- node v22.4.1
- macos sonoma 14.5

Any additional comments?

I discovered this issue while deep-diving on a comparison of the most popular, standalone TS reactivity libs.

During my deep-dive, I wanted to benchmark all of them and created a fork of @milomg's excellent js-reactivity-benchmark, which includes dozens of benchmarks and stress tests.

I'll also note that @vue/reactivity also hangs on the largest dynamic stress test, which I have commented out in src/index.ts at the moment to focus on the simpler cellx stress test.

Given that @vue/reactivity is the only framework to encounter an issue running the cellx benchmark, I figure it could either be a rare bug in @vue/reactivity or a bug in my benchmark adapter. If you see anything that could be improved with the @vue/reactivity adapter for these benchmarks, please let me know.

Thanks! 🙏

@yyx990803 yyx990803 added scope: reactivity regression ❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. labels Sep 16, 2024
@yyx990803
Copy link
Member

Seems like a regression from the 3.5 refactor. 3.4 works fine in this case.

@transitive-bullshit
Copy link
Author

transitive-bullshit commented Sep 16, 2024

I can confirm that https://github.com/vuejs/core/releases/tag/v3.5.6 fixes this issue. 🎉

It also provides a solid perf improvement across all @vue/reactivity benchmark results (averaging ~5.43% speedup):

Before

framework              , test                                                         , time    
Vue                    , avoidablePropagation                                         , 243.72  
Vue                    , broadPropagation                                             , 10.70   
Vue                    , deepPropagation                                              , 80.85   
Vue                    , diamond                                                      , 213.42  
Vue                    , mux                                                          , 106.30  
Vue                    , repeatedObservers                                            , 37.93   
Vue                    , triangle                                                     , 65.06   
Vue                    , unstable                                                     , 2.59    
Vue                    , molBench                                                     , 0.87    
Vue                    , createDataSignals                                            , 8.41    
Vue                    , createComputations0to1                                       , 7.01    
Vue                    , createComputations1to1                                       , 12.90   
Vue                    , createComputations2to1                                       , 10.01   
Vue                    , createComputations4to1                                       , 8.05    
Vue                    , createComputations1000to1                                    , 4.83    
Vue                    , createComputations1to2                                       , 9.32    
Vue                    , createComputations1to4                                       , 8.45    
Vue                    , createComputations1to8                                       , 8.21    
Vue                    , createComputations1to1000                                    , 9.21    
Vue                    , updateComputations1to1                                       , 6.92    
Vue                    , updateComputations2to1                                       , 5.76    
Vue                    , updateComputations4to1                                       , 5.39    
Vue                    , updateComputations1000to1                                    , 3.82    
Vue                    , updateComputations1to2                                       , 6.07    
Vue                    , updateComputations1to4                                       , 5.20    
Vue                    , updateComputations1to1000                                    , 3.84    
Vue                    , 10x5 - 2 sources - read 20% (simple component)               , 240.60  
Vue                    , 10x10 - 6 sources - dynamic - read 20% (dynamic component)   , 241.05  
Vue                    , 1000x12 - 4 sources - dynamic (large web app)                , 7071.24 
Vue                    , 1000x5 - 25 sources (wide dense)                             , 2682.38 
Vue                    , 5x500 - 3 sources (deep)                                     , 133.64  

After

framework              , test                                                         , time    
@vue/reactivity        , avoidablePropagation                                         , 227.00  
@vue/reactivity        , broadPropagation                                             , 10.57   
@vue/reactivity        , deepPropagation                                              , 79.23   
@vue/reactivity        , diamond                                                      , 185.31  
@vue/reactivity        , mux                                                          , 100.12  
@vue/reactivity        , repeatedObservers                                            , 23.05   
@vue/reactivity        , triangle                                                     , 62.81   
@vue/reactivity        , unstable                                                     , 1.53    
@vue/reactivity        , molBench                                                     , 0.50    
@vue/reactivity        , createDataSignals                                            , 9.47    
@vue/reactivity        , createComputations0to1                                       , 3.47    
@vue/reactivity        , createComputations1to1                                       , 10.58   
@vue/reactivity        , createComputations2to1                                       , 10.01   
@vue/reactivity        , createComputations4to1                                       , 9.22    
@vue/reactivity        , createComputations1000to1                                    , 1.76    
@vue/reactivity        , createComputations1to2                                       , 7.65    
@vue/reactivity        , createComputations1to4                                       , 5.26    
@vue/reactivity        , createComputations1to8                                       , 4.53    
@vue/reactivity        , createComputations1to1000                                    , 4.06    
@vue/reactivity        , updateComputations1to1                                       , 3.66    
@vue/reactivity        , updateComputations2to1                                       , 2.35    
@vue/reactivity        , updateComputations4to1                                       , 1.76    
@vue/reactivity        , updateComputations1000to1                                    , 0.85    
@vue/reactivity        , updateComputations1to2                                       , 2.65    
@vue/reactivity        , updateComputations1to4                                       , 1.78    
@vue/reactivity        , updateComputations1to1000                                    , 0.94    
@vue/reactivity        , cellx1000                                                    , 15.36   
@vue/reactivity        , cellx2500                                                    , 37.40   
@vue/reactivity        , cellx5000                                                    , 139.44  
@vue/reactivity        , 10x5 - 2 sources - read 20% (simple component)               , 233.32  
@vue/reactivity        , 10x10 - 6 sources - dynamic - read 20% (dynamic component)   , 223.51  
@vue/reactivity        , 1000x12 - 4 sources - dynamic (large web app)                , 6879.19 
@vue/reactivity        , 1000x5 - 25 sources (wide dense)                             , 2413.88 
@vue/reactivity        , 5x500 - 3 sources (deep)                                     , 122.52  

Note that the after benchmarks include the cellx benchmark tests which were previously failing 🔥

ts-reactivity-benchmark-results-2

Thanks for the quick turnaround @yyx990803 🙏

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. regression scope: reactivity
Projects
None yet
2 participants