Nested Replicator performance issues after PR #6902

o1y commented 1 year ago

Bug description

Since the implementation of PR #6902, which changed the v-if conditions to v-show, there has been a noticeable degradation in performance in our "Page Builder" replicator field, which is using lots of nested replicator fields. I tracked the loading time of my replicator using the browser's performance profiler:

Firefox

v-show: 4.5s
v-if: 1.8s

Chrome

v-show: 2.147s
v-if: 1.197s

The times only represent the Vue/JS loading time, until the Publish form is loaded.

Thank you @jonassiewertsen, for pointing out the pull request!

How to reproduce

I've created a reproduction repository: https://github.com/o1y/statamic-replicator-performance
Just clone it and open the Home Entry in the Pages collection in the control panel.

Logs

No response

Environment

Environment
Application Name: Statamic
Laravel Version: 10.14.1
PHP Version: 8.2.5
Composer Version: 2.4.4
Environment: local
Debug Mode: ENABLED
URL: statamic-replicator-performance.test
Maintenance Mode: OFF

Cache
Config: NOT CACHED
Events: NOT CACHED
Routes: NOT CACHED
Views: CACHED

Drivers
Broadcasting: log
Cache: statamic
Database: mysql
Logs: stack / single
Mail: smtp
Queue: sync
Session: file

Statamic
Addons: 0
Antlers: runtime
Stache Watcher: Enabled
Static Caching: Disabled
Version: 4.9.2 Solo

Installation

Fresh statamic/statamic site via CLI

Antlers Parser

None

Additional details

By the way, it seems that there is a regression regarding the Bard Set v-show. It seems the change from #6902, was probably lost during resolving a merge conflict when merging the 3.3 Branch: https://github.com/statamic/cms/blob/ee63549314abfadbe508e9533dcf10b23dc70cab/resources/js/components/fieldtypes/bard/Set.vue#L38

jacksleight commented 1 year ago

I've also been noticing some significant loading delays with large page builder values that contain many sets, and using v-if over v-show with collapse by default does help a lot.

Of course the reasons for using v-show are detailed in that PR, but since Bard sets are actually (unintentionally) using v-if right now I'm wondering if maybe the original reasons for switching to v-show are no longer a problem? There have been a number of changes/fixes related to tracking hidden field state since then.

If that is the case then could replicators use v-if as well now?

I'm testing v-if on a site with large page builders at the moment and haven't yet run into issues with conditional fields/reordering sets etc.

jesseleite commented 1 year ago

We have to be very careful about not just switching back to v-if though, as v-show was very intentional for fixing a lot of issues around conditional fields and data flow.

If there are performance issues around v-show, I think we should be addressing them in ways that don't cause regressions around all the data flow edge cases we were solving with that v-show change.

Maybe there's more we can do performance-wise to defer slower logic until fields/sets are expanded/shown, without disrupting data flow on save with field conditions, revealers, default field values, etc.

Worth looking into!

jesseleite commented 1 year ago

Maybe there's more we can do performance-wise to defer slower logic until fields/sets are expanded/shown, without disrupting data flow on save with field conditions, revealers, default field values, etc.

^ On thate note, like this kinda stuff https://github.com/statamic/cms/pull/8716 cc/ @jacksleight 💘

Keep in mind, v-if was just hiding our actual performance issues like a big bandaid, while causing other problems in the process.

jacksleight commented 1 year ago

^ On thate note, like this kinda stuff #8716 cc/ @jacksleight 💘

Keep in mind, v-if was just hiding our actual performance issues like a big bandaid, while causing other problems in the process.

Yeah good point! v-if was actually my first attempt at fixing the issue that PR addresses, but it did just feel like a workaround rather than a proper fix. 🙂

statamic / cms