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

Performance regression in fm_renumber in fieldmanager 1.4.0 #827

Closed
jjrdevspace01 opened this issue Feb 23, 2022 · 11 comments · Fixed by #828
Closed

Performance regression in fm_renumber in fieldmanager 1.4.0 #827

jjrdevspace01 opened this issue Feb 23, 2022 · 11 comments · Fixed by #828
Milestone

Comments

@jjrdevspace01
Copy link

I have had to revert to 1.3 on my project, i use some very large Fieldmanager_Groups with display_if and after upgrade fieldmanager 1.4 is unusable. Not sure exactly what has changed but reverting to 1.3 certainly solved it

@mboynes
Copy link
Contributor

mboynes commented Feb 23, 2022

thank you for reporting this @jjrdevspace01! is there any other detail you can provide? screenshots from browser dev tools showing where the cpu is churning would be extremely helpful. we haven't noticed any differences yet, but perhaps we haven't used display_if the same ways you have (and to that point, if you have any code you can share, that would also help tremendously!)

@jjrdevspace01
Copy link
Author

jjrdevspace01 commented Feb 23, 2022

I iteratively build up a set of groups for a site.. its quite hard to give you code but this should give you an idea, but with lor more iterations. its the act of expanding or trying to add a new child that seems to be the problem, sends chrome into 100% cpu, eventually returning a wait or cancel, trying to run a performance report in chrome but its not responsive.
fieldmanager-example-use.txt

@mboynes
Copy link
Contributor

mboynes commented Feb 23, 2022

thanks again, really appreciate your help on this! we'll see if we can replicate and then report back on what we find. assuming we can pin it down and identify the cause, we'll ship a point release to fix it without delay.

@jjrdevspace01
Copy link
Author

@mboynes left it overnight and got a response... looks like querySelectorAll, searching though fm_renumber
Screenshot 2022-02-24 at 08 20 30

@jjrdevspace01
Copy link
Author

jjrdevspace01 commented Feb 24, 2022

for comparison identical action under 1.3

Screenshot 2022-02-24 at 08 28 15

@mboynes
Copy link
Contributor

mboynes commented Mar 1, 2022

@jjrdevspace01 would you be able to try out #828 to confirm the fix works for you?

@dlh01 dlh01 added this to the 1.4.1 milestone Mar 1, 2022
@jjrdevspace01
Copy link
Author

@mboynes thats fantastic yes, the problem is resolved. attached the performance i found testing the hotfix. i would agree with your note that display_if does have some performance issues at scale. Would be amazing if this could be refactored at some point but for now the performance is back to being comparable with 1.3
Screenshot 2022-03-01 at 09 20 58

@mboynes
Copy link
Contributor

mboynes commented Mar 1, 2022

@jjrdevspace01 Glad to hear it, thanks for testing! We'll ship 1.4.1 today.

i would agree with your note that display_if does have some performance issues at scale

For what it's worth, the issue isn't tied to display_if. You encountered it in a field that hinges on display_if because you're using that feature to conditionally render one of many modules, a great use of Fieldmanager in my opinion, and something we at Alley did a lot pre-Gutenberg.

The problem is with how field attributes get modified when adding or sorting groups, and the issue gets very bad if those groups have many fields/subgroups within them. Where display_if factors in is, if you have 20 modules, each with groups of fields, even though only one is rendered at a time, the other 19 are still present in the DOM and are still modified when rendering or sorting. Sorting is actually the worst part; if you add several modules and then drag the last to the first, your browser will probably hang for a few seconds even with the "performant" code.

As an aside, given how many fields you're generating, you may at some point run into a limitation on your web server where some fields are truncated from the request. This is a webserver setting and if I recall correctly, the limit is usually somewhere around 6,000 fields. A lot, but these can add up! If you do ever encounter that, here's a gist that can help you out, it removes all the hidden fields that the server request doesn't need. You'll have to modify it to match your field names, but it should save you a big headache!

@jjrdevspace01
Copy link
Author

yes i have hit that limit, and increased it :) thanks for the gist - i'll try that out - do you have any docs/blogposts on intregration of FM and gutenberg?

@mboynes
Copy link
Contributor

mboynes commented Mar 1, 2022

We don't have any docs; everything (so far) just works with Gutenberg's support for "legacy" meta boxes. That said, we're constantly evaluating new opportunities for how Fieldmanager can integrate with Gutenberg, and as of now, nothing has surfaced where the benefits would far outweigh the costs. We have a GitHub label for Gutenberg issues if you want to follow along, and certainly, if you have any ideas to add, we'd love to hear them!

@mboynes mboynes changed the title fieldmanager 1.4.0 much slower than 1.3.0 Performance regression in fm_renumber in fieldmanager 1.4.0 Mar 1, 2022
@mboynes
Copy link
Contributor

mboynes commented Mar 1, 2022

1.4.1 is now released! Thanks again @jjrdevspace01

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 a pull request may close this issue.

3 participants