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

Fix lag in Jupyter caused by CSS in _repr_html_ #5201

Closed

Conversation

SimonHeybrock
Copy link

@SimonHeybrock SimonHeybrock commented Apr 21, 2021

What

The CSS used by _repr_html_ (for displaying objects in Jupyter) placed font colors in :root as CSS custom properties. This seems to cause lag in notebooks with more than a couple of dozens of cells when running a cell that displays outputs. We observed this on Chrome and Firefox, so it is probably not browser-specific.

To reproduce

  • In a new notebook, create a simple array:
    import xarray as xr
    import numpy as np
    data = np.random.rand(4)
    a = xr.DataArray(data, coords=[np.arange(4)], dims=['x'])
  • From a second cell, display a:
    a
    This is probably fast with no noticable lag.
  • Add 50-100 more cells. They CAN be empty. Run the second cell again. You may notice a small lag before the array is displayed, which can exceed 1 second in some cases (depending on number of cells in the notebook, and probably the type of hardware the browser is running on, it is clearly visible on my 2015 Macbook Pro).
  • Probably other UI interactions such as switching tabs are also affected, but there the effect is not entirely clear.

Fix

  • Set CSS custom properties not in :root but the top-level class xr-wrap.
  • TODO: I think the vscode-dark settings also may need to change, but my understanding of CSS is too limited. Can someone suggest how to fix this or take care of it?

Discussion

Interestingly we ran into this problem independently of xarray: While scipp borrowed an early draft of xarrays's _repr_html_ implementation (thanks!), this was before the color configs were placed into :root --- we just happened to add such a color config independently. Since this was a recent change we managed to find the culprit yesterday (scipp/scipp#1847). And then it occurred to me to check whether xarray has the same problem...

So this makes me think that other projects that define _repr_html_ may well suffer from the same problem. Can we do anything to spread the word, or better, could there be a way to fix this for everyone (in Jupyter)?

Checklist

  • Passes pre-commit run --all-files
  • User visible changes (including notable bug fixes) are documented in whats-new.rst

This seems to cause a major slowdown in notebooks with more than a
couple of dozens of cells.
@max-sixty
Copy link
Collaborator

Thanks @SimonHeybrock ! Tagging @jsignell & @benbovy as the experts!

@jsignell
Copy link
Contributor

I didn't manage to reproduce the lag, but this change seems totally reasonable to me!

@dcherian
Copy link
Contributor

Thanks for the review @jsignell !

I think the vscode-dark settings also may need to change,

Can someone try out this PR in VS Code please?

@max-sixty
Copy link
Collaborator

@SimonHeybrock would you be able to see whether running this with vscode-dark is acceptable? We can merge with a TODO or issue assuming it doesn't make the issue worse in that environment.

@SimonHeybrock
Copy link
Author

I don't have VS code so I can't try, but looking at the CSS I feel that this would actually break the colors there, since I moved the general settings from root into xr-wrap, below the level where the vscode-dark settings are defined. I don't know how to fix this though.

So I would recommend not to merge this unless someone is able to try it out.

@andersy005
Copy link
Member

I don't have VS code so I can't try, but looking at the CSS I feel that this would actually break the colors there, since I moved the general settings from root into xr-wrap, below the level where the vscode-dark settings are defined. I don't know how to fix this though.

@SimonHeybrock, I am going to try this out in VS code and will report back how it goes.

@andersy005
Copy link
Member

andersy005 commented Apr 27, 2021

Here are two screenshots for two themes (using this PR):

Screen Shot 2021-04-27 at 11 09 44 AM

Screen Shot 2021-04-27 at 11 09 11 AM

@andersy005
Copy link
Member

Here's the same notebook (using master branch)

Screen Shot 2021-04-27 at 11 24 11 AM
Screen Shot 2021-04-27 at 11 22 46 AM

@max-sixty
Copy link
Collaborator

Thanks a lot @andersy005

That looks concerning enough we should fix before merging:
image

@SimonHeybrock how would you feel about giving the vscode-dark a swing too?

@SimonHeybrock
Copy link
Author

SimonHeybrock commented Apr 28, 2021

Ok, I tried, but got stuck: I can reproduce the issue in VScode. However, I cannot find a way to inspect the CSS in VScode's Jupyter console. The theme itself is a json and I cannot figure how this is translated into CSS.

We somehow need to detect the theme within xr-wrap and change colors accordingly. That would require checking if a parent/grandparent/... is something defined by VScode, or by checking if custom properties exist. Does someone know how to access the actual HTML/CSS in VScode? In a normal notebook I can use, e.g., the Firefox web-dev tools to do this directly in the browser, but I cannot find anything equivalent in VScode.

@max-sixty
Copy link
Collaborator

I am out of comfort zone with CSS & JS — but does Toggle Developer Tools in VSCode give you access to the DOM?

@SimonHeybrock
Copy link
Author

Cheers, that was what I was looking for!

@SimonHeybrock
Copy link
Author

Or not quite: The DOM seems to end at editor-instance, which is the whole Jupyter part of the Window. I cannot seem to access anything below that using the Developer Tools.

@dcherian
Copy link
Contributor

@fujiisoup added the vscode dark mode support, maybe he has ideas.

@fujiisoup
Copy link
Member

Confirmed that this also breaks the darkmode also in google colab.

@fujiisoup added the vscode dark mode support, maybe he has ideas.

I did it in #4036 but this was actually a workaround and should be improved by an expert.
I'll take a look, but with little hope to fix.

@fujiisoup
Copy link
Member

This

https://github.com/fujiisoup/xarray/blob/6225f158626e75977a0a944fbc09c50769884e35/xarray/static/css/style.css#L5-L29

looks working with a darkmode, but I'm not sure if this solves the original problem.

It looks to me that defining custom properties in html[theme=dark] may cause the same problem.

@max-sixty
Copy link
Collaborator

It would be a shame to lose the benefits of this.

  • It seems like between us we're not going to figure out the dark mode issue. Are there any CSS people out there who could offer guidance?
  • Assuming not, should we merge this knowing dark mode* will look worse? I wrote above we should fix it, but I had thought that was a few extra lines, rather than intractable. I would probably vote to prioritize performance above aesthetics.

(* Did you know that research suggests we're either the same or slightly better at reading light mode?? Is dark mode LARPing?)

@max-sixty
Copy link
Collaborator

Marked this as "plan to merge" — please comment if you think that's a bad tradeoff between performance and dark mode.

@max-sixty max-sixty added the plan to merge Final call for comments label Jun 28, 2021
@fujiisoup
Copy link
Member

I am trying to measure the performance of master, this PR and mine (which fixes this PR to be compatible with dark mode) but couldn't see any big difference in my environment.

What I did in this experiment is to make a notebook with hundreds of empty cells with xarray under these branches.
Refreshed the browser to render the htmls.
Number of cells are the same in all these experiments, but only the xarray branches (and produced html) are different.

Maybe we may need more cells?
Any advice would be appreciated.

css_render.mp4

movie
top left: this branch
top right: mine
bottom left: master

@max-sixty max-sixty removed the plan to merge Final call for comments label Jun 30, 2021
@SimonHeybrock
Copy link
Author

@fujiisoup Maybe I missed it in the video, but did you try if there are differences when running an individual cell, not just when loading the page the first time? My point is:

  • When a page is first loaded, obviously the CSS for everything (all cells) has to be processed. That cannot be changed.
  • When updated a single cell, prior to this branch, it triggered CSS changes for all cells.
  • With this branch, only current cell should be affected.

@fujiisoup
Copy link
Member

did you try if there are differences when running an individual cell, not just when loading the page the first time?

I tried to measure the performance
by running all the cells as shown in the image
image
but I could not find any significant difference.

However, I'm not very confident if this actually measures the css performance.

@SimonHeybrock, do you have any suggestions how to measure the peformance?

@SimonHeybrock
Copy link
Author

Indeed, such timings do not include the CSS timings. The only way I was able to see it was to use the Web Dev tools that come as part of Firefox or Chrome. You should be able to see the timings included there when recording a profile.

@fujiisoup
Copy link
Member

Maybe can we measure the first-loading time?
I observe the first-loading time is very long...
(movie)

The only way I was able to see it was to use the Web Dev tools that come as part of Firefox or Chrome.

Can you tell me more about this? I'll try to reproduce and measure the performance.

repr.mp4

@SimonHeybrock
Copy link
Author

Maybe can we measure the first-loading time?
I observe the first-loading time is very long...

I think this is also a problem, but I believe this is independent and not improved by the CSS changes in this branch. Maybe a Jupyter issue and not related to libraries in use?

The only way I was able to see it was to use the Web Dev tools that come as part of Firefox or Chrome.

Can you tell me more about this? I'll try to reproduce and measure the performance.

So I had used Chrome, open "Developer Tools" > "Performance" tab:

  • start recording a profile
  • run a cell the displays HTML output
  • stop profile

I think I had observed a difference in the "Render" part of the profile, but I cannot check now (I may be able later today when I am back to my main computer).

@fujiisoup
Copy link
Member

Maybe a Jupyter issue and not related to libraries in use?

I see. Indeed, I didn't see any significant difference among branches.

I may be able later today when I am back to my main computer

I tried but I think maybe better to wait for your update.

@SimonHeybrock
Copy link
Author

Before:
image

After:
image

On the top band, I have used the screenshot-timeline to zoom onto the time window where the cell is being executed (marked with [*]), before the new output is displayed. You should be able to see that the time-scale is bastly different.

@benbovy
Copy link
Member

benbovy commented Jul 1, 2021

Here are some profiling results on my laptop (MacOS 11.2, MacBook Pro 13-inch, 2019).

versions:

chrome: 91.0.4472.114
jupyterlab: 3.0.14
xarray: (master)
pandas: 1.2.2

The results below only show the refresh of the ouput cell when I re-execute it (I used the screenshots in chrome's web developer tools to manually set the time span of interest on the profiling timeline).

I've also measured the rendering of a very basic pandas dataframe using

import xarray as xr
import numpy as np
import pandas as pd
data = np.random.rand(4)
a = xr.DataArray(data, coords=[np.arange(4)], dims=['x'])
df = pd.DataFrame({'col': data})

pandas / no additional cell

profile-pandas-no-cell

pandas / many empty cells

profile-pandas-many-cells

xarray / no additional cell

profile-xarray-no-cell

xarray / many empty cells

profile-xarray-many-cells

So it seems to me that it's more a Jupyter notebook issue. The decrease in performance (rendering time) scales pretty much the same for pandas and xarray reprs.

@benbovy
Copy link
Member

benbovy commented Jul 1, 2021

xref: jupyterlab/jupyterlab#8971

@dcherian
Copy link
Contributor

Closing as upstream issue

@dcherian dcherian closed this Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants