corcel / acf

Advanced Custom Fields (ACF) plugin for Corcel
128 stars 100 forks source link

Flexible Content Field speed #24

Open njbarrett opened 7 years ago

njbarrett commented 7 years ago

Hey,

Thanks for developing this amazing package. I'm testing the flexible content field as follows:

        $page = Page::find(17);
        $modules = $page->acf->flexibleContent('modules');

This alone takes 8+ seconds, which I'm assuming is due to the number of queries it has to make. I have 2 layouts, both have repeaters in them, but they contain a few text and image fields. It seems it is returning all of the field data, but doing each field individually and probably generating a lot of queries to get to this.

jgrossi commented 7 years ago

Hi @njbarrett thanks for the comment. Both FlexibleContent and Repeater makes a lot of SQL, even in the original plugin. Our goal is to decrease that, but we need help ;-)

Anyway, 8 seconds is a lot. I don't think this is because of that, of you have a lot of repeater in the same page, for example. If you're using Laravel can you monitor the number of SQL queries using for example Laravel Debugbar?

Cheers, JG.

njbarrett commented 7 years ago

Hey @jgrossi

Thanks for the suggestion. I've run the page in debugbar and found this:

screen shot 2017-03-20 at 10 34 21 am

So 449 statements is a lot, but 348 duplicates is more concerning.

I had a brief look through the queries and the most duplicated query seems to be:

select * frombao_postmetawherepost_idin ('17') . 17 is the post id of the page I am loading.

This query appears many times, so if we can come up with a way to cache this im sure it would speed it up a lot.

I will look at your libraries code when I get some more time - for now I have chosen to cache the returns from the acf functions using Laravels cache drivers.

jgrossi commented 7 years ago

Hey @njbarrett thanks for that!

Yep, we have to remove those duplicated queries. I'm gonna take a look on the code and check where I can "cache" that to void those queries.

Thanks for reporting that. JG

floflock commented 7 years ago

Hey @njbarrett,

is there a current status to cache or prevent so many SQL queries? I ran into the same issue, see below. 🙈

image

Something I saw:

If you are using the Repeater, there is one query executed like:

select * from wp_postmeta where post_id = '216' and (meta_key like 'ingredients0%' or meta_key like 'ingredients1%' or meta_key like 'ingredients2%' or meta_key like 'ingredients3%' or meta_key like 'ingredients4%' or meta_key like 'ingredients5%' or meta_key like 'ingredients6%' or meta_key like 'ingredients7%')

If I manually execute this query, I will retrieve all data in the meta rows. Now, corcel/acf maybe has only to create a mapping to the 'metakey' which is also known at this time. Am I wrong?

jgrossi commented 7 years ago

Hi @florianflock @njbarrett yep! That should be improved a lot. We're doing extra queries, but I think "caching" is not our responsibility here, but better SQL queries. I'm sure we can reduce 4, 5 queries into just one and improve a lot the performance we have.

This is related to #37 too.

floflock commented 7 years ago

Hi @jgrossi,

right, we should concentrate on continuous performance, not only on caching. I cannot help till summer, unfortunately. But I will try to create a pull request on that.

Stay tuned!

jgrossi commented 7 years ago

Hi @florianflock sorry for the late response. I'm gonna try to work on this in the next weeks, and if you can help you're welcome!

jgrossi commented 7 years ago

Improve SQL queries in Repeater and Flexible Content fields

DanDvoracek commented 7 years ago

What is the official way to get contents from a flexible? Can't find any ref to that even though it seems implemented.

I tried the @njbarrett solution ($modules = $page->acf->flexibleContent('modules');) as my flexible is named Modules but it keeps returning an empty array/object... Any hints? Is there a proper documentation I didn't find yet? Thanks!

jgrossi commented 7 years ago

@DanDvoracek there's no official documentation about all fields for now. The code itself is very simple and easy to understand. You're welcome to send a PR ;-)

jgrossi commented 7 years ago

@DanDvoracek and about getting the flexibleContent you're doing it right. If the call is returning an empty result is a good opportunity to debug your code and maybe fix a bug in the code :-) thanks!

DanDvoracek commented 7 years ago

Hmmm... @jgrossi I'm simply testing the whole thing for now so I don't have ANY complexity on the Laravel side.

Only thing that I do is the following:

// PagesController
$pages = Page::type('page')->status('publish')->get();
return view('frontend.pages', ['pages' => $pages]);

Here is the model I use... Simply extending the Corcel\Model\Page .

<?php

namespace App;

use Illuminate\Database\Eloquent\Model;

use Corcel\Model\Page as Corcel;

class Page extends Corcel
{

}

Anything I'm doing wrong at that level?

jgrossi commented 7 years ago

@DanDvoracek no, everything is correct. You can simplify:

$pages = Page::published()->get();
return view('frontend.pages', compact('pages'));

If you have published pages in the WP database this must work.

DanDvoracek commented 7 years ago

Right I changed the way I query and return the data in the view.

Unfortunately,no luck and still nothing :/ I'm gonna dive into that a bit more.

jgrossi commented 7 years ago

@DanDvoracek DId you configure the Laravel database connection? Do you have any data in the database?

DanDvoracek commented 7 years ago

@jgrossi Of course I did ;) Everything seems fine on the connection side as I was able to output the data I needed. I could even work with repeaters but the flexible content is not working at all for me.

DanDvoracek commented 7 years ago

@jgrossi Ok I got it. Nothing wrong with your plugin. I was using clones inside my flexible.. I suppose that has broken smth in the transaction (I can see this type isn't supported). Is there any plan to support that type of content from acf too ?

Thanks anyway ;)

EDIT: clones work using @drosendo solution here: https://github.com/corcel/acf/issues/49 . Not really Corcel plugin related but I am able to get all the data no matter what config I have in my Wp backend.. And I even do less queries in my frontend, so I guess I'm happy with that, for now.

Thanks!

njbarrett commented 7 years ago

Hi @DanDvoracek maybe also look at my old PR for clone fields https://github.com/corcel/acf/pull/25