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

Editor search should match on notebook output by default #174969

Closed
Tracked by #175778
joyceerhl opened this issue Feb 21, 2023 · 7 comments
Closed
Tracked by #175778

Editor search should match on notebook output by default #174969

joyceerhl opened this issue Feb 21, 2023 · 7 comments
Assignees
Milestone

Comments

@joyceerhl
Copy link
Collaborator

Testing #174637

image

This repros for me with or without experimental scrollable output support enabled.

@amunger
Copy link
Contributor

amunger commented Feb 21, 2023

currently you need to specifically enable searching in cell output, but it might be a good idea to enable that when the focus was in the output when the find widget was opened.

image

cc @rebornix

@joyceerhl
Copy link
Collaborator Author

Would it make sense to enable searching in cell output by default (or have we received user feedback that it's noisy or undesirable to show those results)?

@amunger
Copy link
Contributor

amunger commented Feb 21, 2023

It's a performance concern, especially now that outputs can be much longer.

@amunger amunger added bug Issue identified by VS Code Team member as probable bug notebook-output labels Feb 21, 2023
@isidorn
Copy link
Contributor

isidorn commented Feb 22, 2023

+100 for enabling this by default. If it is a performance concern, could you provide more details why?
I am curious since our editor supports find / replace even when it open 100K+ lines of content.

fyi @rebornix since he might have advice on how Find works in huge editors

@amunger amunger changed the title Editor search doesn't match on notebook output contents Editor search should match on notebook output by default Mar 1, 2023
@rebornix
Copy link
Member

rebornix commented Mar 8, 2023

The major concern we have for turning on find in output by default is performance since it requires all outputs to be rendered before searching keywords. I used PythonDataScienceHandbook as a good real world sample to test how much will be spent in rendering all outputs on the reveal of the find widget.

Most of notebooks in PythonDataScienceHandbook has cells between dozens to hundreds and they are good mixes of markdown cell and code cells with outputs. Rendering a cell output ~= 1 to 3ms depending on the output type (text or rich output like image). With a notebook that contains 100 cells, it takes around 200ms to 300ms to finish the full rendering of outputs. This process will block the renderer process for a while to prepare and send the output data to the webview, and then the webview will block for a while to render all outputs. A flame chart for this particular notebook is as below

image

To avoid the webview to be blocked for too long (which will affect the scrolling or any other user interactions), we want to request the output rendering when Idle, not only checking if the renderer process is Idle, but also if the webview is Idle. Another approach is only giving the output warmup task a small of time frame, for example, 100 ms per 5 seconds, to ensure it's not adding too much CPU cycles for just viewing the notebook.

@rebornix rebornix added this to the March 2023 milestone Mar 8, 2023
@rebornix rebornix added polish Cleanup and polish issue and removed bug Issue identified by VS Code Team member as probable bug labels Mar 8, 2023
@rebornix
Copy link
Member

rebornix commented Mar 8, 2023

Based on the design of https://www.w3.org/TR/requestidlecallback/#idle-periods, we need to do runWhenIdle in both webview and renderer processes

image

@amunger
Copy link
Contributor

amunger commented Mar 20, 2023

completed

@amunger amunger closed this as completed Mar 20, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants