2

By inefficient I mean, activating the code makes the page hang for 20+ seconds.

To set the scene, I currently have an HTML table that looks like the following. It can be fairly big, easily 1,000-1,500 rows and 40 columns wide. It is generated from Python/Flask as a static HTML table and then javascript takes over to allow the users to filter out and sort rows. I do use the jquery tablesorter widget to allow users to sort rows by whatever column they wish.

The table itself has a structure like:

<table id="myTablePlayers" class="tablesorter table table-striped table-bordered table-hover" style="overflow: visible">
    <thead>
        <tr>
          <th>...</th>
          <th>...</th>
          <th>...</th>
          <th>...</th>
          ...
          <th>...</th>
        </tr>
    </thead>
    <tbody>
        <tr class="playerData">
            <td>...</td>
            <td>...</td>
            <td>...</td>
            <td>...</td>
            ...
            <td>...</td>
        </tr>
        ...
    </tbody>
</table>

The filters that users are given are as follows:

  • minimum GP - input field that removes all rows less than the user's input in a specific column
  • teams - select field (text) that removes all rows that does not match in a specific column
  • position - select field (text) that removes all rows that does not match in a specific column
  • age - input field that removes all rows less than the user's input in a specific column (e.g. if enters 20, it will keep all rows with the age in the range [20.0, 21.0)

The javascript/jquery I have written and is likely the culprit is as follows:

function autoRank() {
    // auto number
    rank = 0;
    $("#myTablePlayers .playerData").each(function() {
        if ($(this).css("display") != "none") {
            rank++;
            $(this).find('td').eq(colRank).text(rank);
        }
    });
}

function filterTable() {
    // Need some error checking on input not number
    minGP = $("#mingp").val()
    teams = $("#teamFilter").val()
    position = $("#position").val()
    age = $("#age").val()

    $("#myTablePlayers .playerData").show();

    $("#myTablePlayers .playerData").each(function() {
        toHide = false;

        if (teams != "") {
            if ( !$(this).find('td').eq(colTeam).text().toUpperCase().includes(teams.toUpperCase())) {
                toHide = true;
            }
        }

        if ( Number($(this).find('td').eq(colGP).text()) < minGP ) {
            toHide = true;
        }

        if (position != "") {
            if (position == "D") {
                if ($(this).find('td').eq(colPos).text().indexOf("D") == -1) {
                    toHide = true;
                }
            } else if (position == "F") {
                if ($(this).find('td').eq(colPos).text().indexOf("D") != -1) {
                    toHide = true;
                }
            } else if ( $(this).find('td').eq(colPos).text() != position) {
                toHide = true;
            }
        }

        if (age != "") {
            column = Number($(this).find('td').eq(colAge).text())
            age = Number(age)
            if (  column < age || column >= age+1  ) {
                toHide = true;
            }
        }

        if (toHide == true) {
            $(this).hide();
        }

    });

    autoRank();
}

$("#teamFilter").on('change', filterTable);

$("#mingp").on('change', filterTable);

$("#position").on('change', filterTable);

$("#age").on('change', filterTable);

What is the inefficiency in this code that makes the browser hang? What should I be changing to make it efficient?

I looked at Google but jquery table filter plug ins do not give me the ability to filter rows based on specific columns based on specific inputs as outlined above (e.g. https://www.sitepoint.com/12-amazing-jquery-tables/).

Reily Bourne
  • 5,117
  • 9
  • 30
  • 41
  • 1
    Did you benchmark putting that many elements into the DOM without and javascript? I think it might stressing the browser just in terms of HTML elments. – Victory Nov 27 '16 at 23:52
  • 1
    This might help increasing the performance of your code. [What is fastest children() or find() in jQuery?](http://stackoverflow.com/questions/648004/what-is-fastest-children-or-find-in-jquery) – legitghost Nov 27 '16 at 23:52
  • Take a look at www.datatables.net jquery plugin Its much more robust than the jquery widget. – Bindrid Nov 28 '16 at 00:49
  • Take a look also at [caching your elements](http://stackoverflow.com/a/23743458/1022914). – Mikey Nov 28 '16 at 01:15
  • I would use server side pagination and search/filter and not render everything to the DOM at once. Wouldn't paginating at 50 or 100 rows be enough for the user? – jamescharlesworth Nov 28 '16 at 02:00
  • In working with the solutions suggested below it seems to be a Safari specific issue as the code executes instantaneously on Chrome/Firefox. Safari seems to take ~20 seconds to loop through ~1,000 rows in the main loop in `FilterTable()` – Reily Bourne Nov 29 '16 at 05:53

2 Answers2

3

Currently your code works like this:

  • iterate all rows
  • then for each row:
    • successively for each not-empty filter, look for all its child columns
    • then isolate the involved column and get its value

Only regarding the above exposed mechanism and using some numbers you cited it means that, with a unique simple filter like "teams" you actually touch 40000 columns (1000 rows * 1 filter * 40 columns).
But if 2 filters are not-empty it immediately grows to 80000 columns touched, and so on.

This is obviously a first realm where to find a way to work faster, with a pretty simple change like this:

  • iterate all rows
  • then for each row:
    • look for all its child columns
    • successively for each not-empty filter, then isolate the involved column and get its value

The involved part of code becomes:

$("#myTablePlayers .playerData").each(function() {
    var toHide = false,
        columns = $(this).find('td');

    if (teams != "") {
        if ( !columns.eq(colTeam).text().toUpperCase().includes(teams.toUpperCase())) {
            toHide = true;
        }
    }
    // ...same for next filters

This way, we already get rid of multiplying the column touches by the number of not-empty filters.

But we can go further...
In the current situation, each execution actually touches all the cells of the table, while at most 4 columns are involved (for the 4 filters). So we might try to find a way to reduce the total number of touched columns from 40000 to 4000!

This can be achieved by affecting a distinguishing class (say the filter-name) to these involved columns, so we might change the code like this:

$("#myTablePlayers .playerData").each(function() {
    var toHide = false,
        classTeam = '.colTeam',
        classGP = `.colGP`,
        classPos = `.colPos`,
        classAge = `.colAge`;

    if (teams != "") {
        if ( !$(classTeam, this).text().toUpperCase().includes(teams.toUpperCase())) {
            toHide = true;
        }
    }
    // ...same for next filters

Maybe there is an issue with this:

It is generated from Python/Flask as a static HTML table

which means you don't have hand on the generated table.
If so, you can merely add the following to affect the class names just after the page load:

$(document).ready(function() {
    $("#myTablePlayers .playerData").each(function() {
        $('td', this).eq(colTeam).addClass(classTeam);
        $('td', this).eq(colGP).addClass(classGP);
        // ...
    }
}

But actually it might be improved another way (then the previous suggestion becomes useless), using a totally different method.
Since the table is static, we can act just after the page load (so only once) to prepare the needed data for a more direct access when filtering happens.

We can pre-register all the involved columns for each filter:

$(document).ready(function() {
    var teamCols = $(),
        GPCols = $(),
        posCols = $(),
        ageCols = $();
    $("#myTablePlayers .playerData").each(function() {
        var columns = $(this).find('td');
        teamCols.add(columns.eq(colTeam));
        GPCols.add(columns.eq(colGP));
        posCols.add(columns.eq(colPos));
        ageCols.add(columns.eq(colAge));
    }
}

Then the filter can process directly addressing the involved columns. BTW we can also immediately hide their parent (this was already possible in the original version):

if (teams) {
    teamCols.each(function() {
        if (!this.innerHTML.toUpperCase().includes(teams.toUpperCase())) {
            $(this).parent().hide();
        }
    }
}

This is written a bit quickly, so it might contain some flaws, and also might be yet improved...

cFreed
  • 4,404
  • 1
  • 23
  • 33
  • Thanks, your solutions have helped increase but it's still a noticeably large delay. The more I look at it the more it seems to be a Safari specific issue. Executing the code per your third example `teamCols.each(function() {...}` takes ~20 seconds for 1,000 rows on Safari, but near instantly on Chrome and Firefox. – Reily Bourne Nov 29 '16 at 05:52
  • 1
    @ReilyBourne Oh! Then it seems to be pretty different than "look for performance improvement" now. So I suggest you post a new, distinct question about that, (something like "Why this works instantly on Chrome/FF while taking ~20 seconds on Safari?"). Note that it'd be posted on SO rather than CR! Already for now, the first direction I'd target to work on it is to put "time-trace points" in the code to isolate some parts, compare time ellapsed between Safari and, say FF. Then change the time-trace points localization to make isolated parts more atomic, and so on... – cFreed Nov 29 '16 at 17:27
  • Thanks I did post an updated question earlier. http://stackoverflow.com/questions/40870513/jquerys-each-slower-on-safari-than-chrome-firefox – Reily Bourne Nov 29 '16 at 21:52
1

You can improve your code by caching your re-used elements, and doing less iterations. Currently, you do three iterations over the records that can be condensed into one.

Here's how I would do it (untested):

$(function () {
    // cache the elements that you will be re-using
    var $minGP = $("#mingp"), 
        $team = $("#teamFilter"), 
        $position = $("#position"),
        $age = $("#age");

    var $rows = $("#myTablePlayers .playerData");

    function filterTable() {
        // Need some error checking on input not number
        var minGP = $minGP.val(),
            team = $team.val(),
            position = $position.val(),
            age = $age.val();

        // set the rank as we loop (instead of having a separate iteration for ranking)
        var rank = 0;

        // when you use show() and .each(), you are iterating over all the rows twice 

        // instead loop once
        $rows.each(function() {
            // cache current row as it will be re-used
            var $row = $(this);
            // set flag to show or hide
            var display = true;

            if (teams != "" && 
                !$row.find('td').eq(colTeam).text().toUpperCase().indexOf(teams.toUpperCase()) > -1) {
                display = false;
            }
            if (Number($row.find('td').eq(colGP).text()) < minGP) {
                display = false;
            }
            if (position != "") {
                if (position == "D" && 
                    $row.find('td').eq(colPos).text().indexOf("D") == -1) {
                    display = false;
                } else if (position == "F" && 
                    $row.find('td').eq(colPos).text().indexOf("D") != -1) {
                    display = false;
                } else if ($row.find('td').eq(colPos).text() != position) {
                    display = false;
                }
            }
            if (age != "") {
                column = Number($row.find('td').eq(colAge).text())
                age = Number(age)
                if (column < age || column >= age + 1) {
                    display = false;
                }
            }
            // hide or show row
            $tr.toggle(display);
            // set rank number
            if (display) {
                $row.find('td').eq(colRank).text(rank++);
            }
        });
    }
    // attach change event handler
    $minGP.add($team).add($position).add($age).change(filterTable);
});

This might speed up your code by few seconds, but ultimately it depends on how much and how big your data is.

Mikey
  • 6,728
  • 4
  • 22
  • 45