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

Custom borders plugin hook calls grow exponentially with data size #6052

Closed
warpech opened this issue Jun 13, 2019 · 1 comment

Comments

@warpech
Copy link
Contributor

commented Jun 13, 2019

Description

I was investigating why Handsontable becomes very slow with grids that contain multiple borders. It seems that the code in Custom Borders plugin causes calls to the plugin hook afterDrawSelection to grow expotentially as the number of cells or borders grow. This causes CPU to clog for many seconds.

Steps to reproduce

With the following code:

<script src="dist/handsontable.full.js"></script>
<link href="dist/handsontable.full.css" rel="stylesheet" media="screen">
<div id="example1"></div>
<script>
    var hot = Handsontable(document.getElementById('example1'), {
        data: Handsontable.helper.createSpreadsheetData(2, 2),
        fixedColumnsLeft: 2,
        fixedRowsTop: 2,
        customBorders: [
            {
                row: 0, col: 0,
                left: { width: 2, color: 'red' }
            }],
        afterDrawSelection: function () {
            console.count("`afterDrawSelection` hook was called so many times");
        }
    });

You expect a small 2x2 grid with one border on the left of the first cell. However, running the above example renders this in the console:

afterDrawSelection hook was called so many times 16

Now, change the size of the grid from 2x2 to 50x10 and you will get:

afterDrawSelection hook was called so many times 624

Why? The number of calls should not change.

Now, add a second custom border to the configuration:

        customBorders: [
            {
                row: 0, col: 0,
                left: { width: 2, color: 'red' }
            },{
                row: 0, col: 1,
                left: { width: 2, color: 'red' }
            }],

afterDrawSelection hook was called so many times 1872

You get the point... For a grid size 50x10 with 5 left borders, you get:

afterDrawSelection hook was called so many times 9360

Demo: https://jsfiddle.net/uvyzqxa0/2/

Want to know what it get's weirder? Range custom borders. For a grid size 50x10 with 1 ranged left+top+right+bottom borders, you get:

afterDrawSelection hook was called so many times 34320

For a grid size 50x10 with 1 ranged left+top+right (without bottom!) borders, you get even more:

afterDrawSelection hook was called so many times 39936

Demo: https://jsfiddle.net/uvyzqxa0/1/

Solution

I think I have a solution. I will be posting it as a PR soon.

Your environment

  • Handsontable version: 7.1.0
  • Browser Name and version: Ch 74
  • Operating System:

warpech added a commit that referenced this issue Jun 16, 2019

fix custom border performance problems (#6052)
- extended the API of EventManager to include a new boolean parameter that removes only the event listeners that were added using the specific instance of EventManager
   - used this new API in the Overlays class instead of the custom solution already in place there
- added "destroy" method in Border class that removes event manager callbacks to free-up DOM nodes and removes elements from DOM
- added "destroy" method in Selection class that destroys the Borders associated with the Selection instance
- call "destroy" on Selection before splicing (deleting) it from configuration
- changed initialization of CustomBorders from "afterInit" to "init" to avoid need for forced re-render on initialization
- increment "add" variable in "prepareBorderFromCustomAddedRange" only when the border is not empty
- adjusted assertions in test suite, because for some assertions the fixes changed the expected value to more desirable
- removed forced re-renders ("wt.draw(true)") from CustomBorders
- in "hideBorders", delete borders from screen by executing "clearNullCellRange", to compensate for the lack of forced re-render
- marked more potential perf improvements as "TODO" comments in code
@warpech

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

Fixed in #6059, Merged to develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.