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

Carriage return perf #6304

Merged
merged 4 commits into from May 4, 2019
Merged

Conversation

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 4, 2019

Fixes #4202, for real this time. Boy, it's kind of tricky to debug something for which the test case locks up the browser. The problem was essentially a bad cached value. We were overwriting carriage returns and backspaces correctly, but then not updating the cached value of the overwritten output stream. This meant that we were doing the same work, over, and over, and over, in a quadratically growing fashion.

The real fix for this is a one-liner in b899ab0 . The rest of this PR is just some cleanup.

References

#4202.

Code changes

Restructuring of private functionality.

User-facing changes

None, except that the browser won't lock up when using progress bars.

Backwards-incompatible changes

Removal of IModelDB from output area input options. So far as I know, this was only used by the now-broken Google RTC.

@ian-r-rose ian-r-rose added this to the 1.0 milestone May 4, 2019
@ian-r-rose ian-r-rose requested a review from jasongrout May 4, 2019
@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented May 4, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder


// If we are given a IModelDB, keep an up-to-date
// serialized copy of the OutputAreaModel in it.
if (options.modelDB) {
Copy link
Contributor

@jasongrout jasongrout May 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also delete the modelDB option in the options interface?

Copy link
Member Author

@ian-r-rose ian-r-rose May 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I guess that's a breaking change, but one that I am pretty sure was used nowhere but in the context of Google RTC.

Copy link
Contributor

@jasongrout jasongrout May 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you note it above in the breaking changes section?

Copy link
Contributor

@jasongrout jasongrout May 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, you already did. Thanks!

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 4, 2019

Nice, the progress bar example in #4202 is now really smooth!

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 4, 2019

Looks good to me.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 4, 2019

Thanks! Let's merge after tests pass.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 4, 2019

Looks like the linux integrity test failed because a timeout to yarn? I tried restarting it, but it seems to ignore my efforts to rerun.

@ian-r-rose ian-r-rose force-pushed the carriage-return-perf branch from 738653f to e7774e9 May 4, 2019
@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented May 4, 2019

Not sure if it was the cause of the timeout, but there was a real mistake I just fixed. Let's see if that changes things.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 4, 2019

Looks like the linux python test is having an error installing:

error Couldn't find package "@jupyterlab/rendermime-interfaces@^1.3.0-alpha.6" required by "@jupyterlab/docregistry@^1.0.0-alpha.6" on the "npm" registry.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented May 4, 2019

Maybe npmjs.org is having a hiccup?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 4, 2019

Maybe npmjs.org is having a hiccup?

Or the linux images, since it seems like Windows was fine.

@jasongrout jasongrout merged commit 88e20b0 into jupyterlab:master May 4, 2019
9 checks passed
@vidartf
Copy link
Member

@vidartf vidartf commented May 4, 2019

Will the IModelDB things affect the current RTC work?

@cupdike
Copy link

@cupdike cupdike commented May 4, 2019

Thanks for the efforts on this one--cheering you on from the sidelines (as I'm sure others are too). REALLY looking forward to being able to reliably do long running deep learning training runs in JupyterLab.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented May 4, 2019

@vidartf that's a good point. My hope had still been to remove it entirely (and this particular application of it has always smelled off), but if you feel it would still be useful as a bridge, we can restore it here.

@blink1073
Copy link
Member

@blink1073 blink1073 commented May 5, 2019

I'm going to backport that first commit and make a 0.35.x release tomorrow. Thanks @ian-r-rose, this one was a 🐻!

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented May 5, 2019

Awesome, thanks @blink1073. Note that the second commit contains the fix.

@blink1073
Copy link
Member

@blink1073 blink1073 commented May 5, 2019

👍

@lock lock bot locked as resolved and limited conversation to collaborators Aug 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants