function render makes website 500% slow! can anyone fix that please?

Question

Function render makes website 500% slow! Can anyone fix that please ? Someone told me :

because it sends a database request on each iteration of the loop (it's not the only problem with this chunk of code but it's the most taxing one)

Yes I understand what that means. His way is:

you need to get all of the data before you start building the menu, then you just insert the data instead of requesting more data on each iteration

But i don't know how i must do it!

<?php
$menu_html='';
function render_menu($parent_id,$actmenuid)
{
    $obj = new Database(); 
    $con = $obj->dbconnectt();
    global $menu_html;
    $result=mysqli_query($con, "select * from tbl_menu where parent_id='$parent_id'");
    
    if(mysqli_num_rows($result)==0) return;

    if($parent_id==0){
        $menu_html.='<ul class="topnav">';
    }else{
        $menu_html.='<ul>';
    }
    while($row=mysqli_fetch_array($result)) {
        $childnum = $obj->recordcount("SELECT * FROM tbl_menu WHERE parent_id='".$row['id']."'");
        if($childnum == 0){ 
            $linkvalue='/category/'.$row['id'].'.html';
        } else{ 
            $linkvalue='#';
        }
        
        if($row['id']==$actmenuid && $actmenuid !=NULL){
            $actv='class="active"';
        }else{
            $actv='';
        }
        
        $menu_html.='<li '.$actv.'><a href="'.$linkvalue.'">'.$row['title'].'</a>';
        render_menu($row['id'],$actmenuid);
        $menu_html.='</li>';
    }
    $menu_html.='</ul>';return $menu_html;
}
if($isDsh==false){
    echo render_menu(0,$actmenuid);
}
?>

It would be useful to see that database that is being processed here — RiggsFolly, Jul 05 '20 at 00:49
If your code delivers the correct output (works as desired), but needs to be refined, then your question qualifies to be answered by the smart folks at [codereview.se]. — mickmackusa, Jul 05 '20 at 01:22
Is the database properly indexed? 1000 is nothing for mysql; it can handle millions even. — Funk Forty Niner, Jul 05 '20 at 01:44
https://drive.google.com/drive/folders/1-UGVn3_hCKKih_AtEaCAdrjZY8Pt65GI?usp=sharing — mahdi jn, Jul 05 '20 at 01:58
You didn't answer what I asked earlier. If you have additional also, that should be in the question, and not as an external source. — Funk Forty Niner, Jul 05 '20 at 02:23
yes the database indexed . Sorry i didn't see(understand) your question. — mahdi jn, Jul 05 '20 at 02:30

score 0 · Answer 1 · answered Jul 05 '20 at 01:00

0

Depending on how many records you have, try removing this query from inside the loop since it's running for every record on the first query.

$childnum = $obj->recordcount("SELECT * FROM tbl_menu WHERE parent_id='".$row['id']."'");

Change it a single query like this where it returns counts for each parent idea, and place it outside of the loop:

$parentcount = mysqli_query($con, ("SELECT parent_id, count(*) FROM tbl_menu GROUP BY parent_id");

There may be other issues, so please post the database structure and number of records that you're working with too.

answered Jul 05 '20 at 01:00

Spudly

310
1
2
10

more than 1000 records – mahdi jn Jul 05 '20 at 01:23
@mahdijn It's hard to read this db structure. In the future, update the question to show stuff like this in code format. Like others have mentioned, 1,000 records is not a lot, so try moving your second query outside the loop (use any of the answers posted here). All three answers are addressing the problem of having a query inside your while loop, so any of these approaches should fix the problem. – Spudly Jul 05 '20 at 02:09

EhsanT · Answer 2 · 2020-07-05T02:14:20.227

basically you need to do what @spudly has suggested. But there is a small catch in his solution which depending on the number of the rows in yous tbl_menu table you may use a big chunk of memory to fetch all the records. you can optimise it more with using his solution but changing the query to:

select
    parent_tbl_menu.id,
    count(child_tbl_menu.id) as cnt
from
    tbl_menu as parent_tbl_menu
left join
    tbl_menu as child_tbl_menu
on parent_tbl_menu.id = child_tbl_menu.parent_id
where
    parent_tbl_menu.parent_id = ?
group by
    parent_tbl_menu.id

This way you will only fetch the child records of a specific parent. And please consider using prepared statements as your code has sql injection vulnerability.

mickmackusa · Answer 3 · 2020-07-05T01:42:23.003

Don't make recursive queries.

Having "more than 1000" rows is not too big. You can simply call everything from the table into php, then perform the recursive html build in php this will have a memory overhead, but far less processing overhead because you only ever make one trip to the db.
Alternatively (when your db table is prohibitively large), you should avoid gathering rows unnecessarily by adding a new column. The new column will store all "descendants" for the respective row when the row is INSERTed or update it when it is UPDATEd. Then you only need to reference this column when needing to call specific rows. In other words, do the recursive processing only once (when writing to the db) AND not when needing to display the data. This will, again, produce a finite result set in one query which can then be recursively traversed to build the desired output.

score 0 · Answer 4 · answered Jul 05 '20 at 05:26

Connect (from PHP to MySQL) only once for the entire web page.
Don't put a SELECT inside a loop if you can do all the work in a single SELECT, such as with a JOIN. (Exception: A "hierarchical" table needs the nested SELECT. Exception to the exception: MySQL 8.0 and MariaDB 10.2 can do it with a "recursive CTE".)
Don't fetch all the columns (SELECT *) when all you want it is a recordcount. Instead, SELECT COUNT(*) ... and use the number returned.
1000 of anything is probably excessive for a web page. Re-think the UI.

function render makes website 500% slow! can anyone fix that please?

4 Answers4