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

Initialization performance in large table #144

Open
hpcsc opened this issue Jul 31, 2014 · 6 comments
Open

Initialization performance in large table #144

hpcsc opened this issue Jul 31, 2014 · 6 comments

Comments

@hpcsc
Copy link

hpcsc commented Jul 31, 2014

Hi,

I have a reporting table that use treetable, and with the data is average (hundreds of rows, around 50 columns), the initialization of treetable is quite slow. As in the picture below, it takes 4 seconds +

treetable-before

After debugging and profiling the code, I found out that it's actually the checking whether the row is visible using jquery selector is slow (in Node.prototype.expand(), the checking of $(this.row).is(":visible"))
After I replace that checking with !this.row[0].hidden, the performance is greatly improved (200 ms +)

Not really sure this breaks anything, but so far it's working for me. So I'm just reporting it and if the author can help to check whether it's a valid fix

treetable-after

@acacha
Copy link
Contributor

acacha commented Oct 19, 2015

@hpcsc I will try this! If I see any errors i wil say to you

It has been a lot of time this issue was posted so maybe you have some updated info about this?

acacha added a commit to acacha/jquery-treetable that referenced this issue Oct 19, 2015
Pull request for issue ludo#144
@acacha
Copy link
Contributor

acacha commented Oct 19, 2015

Or maybe @ludo will apply a change I have done a Pull request #179

@djlerman
Copy link

Will this be applied to a new available version soon or should I try and apply this fix to my local installed version?

@ludo
Copy link
Owner

ludo commented Nov 18, 2015

I would love to accept this change, but as I mentioned in the related merge request (#179) a test fails when this change is applied. I currently do not have the time to invest a lot of time in this but if you are able to figure out why this test fails I will gladly accept the change. (perhaps the test is invalid, or maybe there is an actual issue with the change)

@djlerman
Copy link

I wish I knew what that meant. I don't know anything about the tests. Is there a way via GitHub to start bounties for actions?

@pete-woods
Copy link

I added a new pull request (#210) that fixes the bug, while keeping the tests passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants