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

investigate performance of Data Viewer #2962

Closed
kevinushey opened this issue Jun 8, 2018 · 5 comments
Closed

investigate performance of Data Viewer #2962

kevinushey opened this issue Jun 8, 2018 · 5 comments
Assignees
Labels
data viewer developer
Milestone

Comments

@kevinushey
Copy link
Contributor

@kevinushey kevinushey commented Jun 8, 2018

A number of users have reported issues with sluggishness in the data viewer, even in v1.2:

https://community.rstudio.com/t/rsstudio-1-1-perfomance-issues/1915/17

We should see if there are any simple reasons why performance could be so poor (perhaps a poorly-interacting combination of CSS styles from the theme?)

@kevinushey kevinushey added this to the v1.2 milestone Jun 8, 2018
@jmcphers
Copy link
Member

@jmcphers jmcphers commented Jun 11, 2018

The example has 100 columns. Unfortunately the DataTables fixed header doesn't deal well with wide data; we're tracking this in #1771.

@kevinushey
Copy link
Contributor Author

@kevinushey kevinushey commented Jun 11, 2018

The point here is that this example:

test.mat <- matrix(data=runif(2000000,0,1),nrow=20000,ncol=100)
View(test.mat)

has acceptable performance in RStudio v1.0, even when viewing all 100 columns. However, performance degrades severely in RStudio v1.1 and v1.2. That is, this is a performance regression independent of the expansion on the maximum number of columns.

This issue is intended to be different in scope from #1771, where all solutions posed are expensive. Rather, I want to try and find the cause of the regression in rendering performance if possible -- I suspect it's due to the extra CSS classes due to theming.

@kevinushey kevinushey reopened this Jun 11, 2018
@kevinushey
Copy link
Contributor Author

@kevinushey kevinushey commented Jun 11, 2018

It looks like most of the performance degradation is a result of the positioning of the resizer elements. In particular:

  1. The performance hit goes away if we render text nodes directly, rather than wrapping in classes at

    var renderCellClass = function (data, type, row, meta, clazz) {
    var title = null;
    if (typeof(data) === "string")
    title = data.replace(/\"/g, "&quot;");
    return '<div class="cell">' +
    '<div class="' + clazz + '" ' +
    (title !== null ? 'title="' + title + '"' : '') +
    '>' +
    renderCellContents(data, type, row, meta) + '</div>' +
    '<div class="resizer" data-col="' + meta.col + '" /></div>';
    };
    .

  2. Performance is also much improved if we remove the overflow: hidden style on the cell, remove the position: absolute style on the resizer, and remove the position: relative attribute on cells.

Even still, it's a bit of a shame as rendering of the above matrix is buttery smooth on v1.0, but still a little choppy on v1.2.

One other data point: viewing this matrix is nice and smooth when using Chrome itself (+ ant devmode), so it seems like there could be some Chromium switch that we could potentially flip to improve performance here as well. (It's also possible that the Qt glue to Chromium is giving us the performance headache as well, though)

Column resize is definitely slow in both RStudio and Chrome, though.

@jmcphers
Copy link
Member

@jmcphers jmcphers commented Jun 11, 2018

Thanks, appreciate the additional clarity.

Ideally we could fix this by drawing the resizer elements differently, but I'm not sure whether we can do that in a way that's more performant.

Perhaps for 1.2 we could mitigate this by not drawing these elements all the time. Proposal:

  1. Add a "resize mode" latching button to the toolbar
  2. When the button is unlatched (the default), we render only the cell contents themselves
  3. When the button is latched, the table is rendered with all of the resize elements present, and emphasized.

@kevinushey
Copy link
Contributor Author

@kevinushey kevinushey commented Jun 11, 2018

That sounds like the right way forward for v1.2.

As an aside: is there any mechanism by which we could draw a 'layer' over the rendered table, and draw our own resize divs at the borders of each column? This way we could draw a single resizer per column rather than embedding and positioning one resizer for each cell. (This might be too much for v1.2 though)

@ronblum ronblum added the developer label Jun 13, 2018
@kevinushey kevinushey self-assigned this Jun 21, 2018
@rich-rstudio rich-rstudio self-assigned this Feb 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data viewer developer
Projects
None yet
Development

No branches or pull requests

4 participants