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 01:46
@jupyterlab-dev-mode
Copy link

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

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

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

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

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

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

@jasongrout
Copy link
Contributor

Looks good to me.

@jasongrout
Copy link
Contributor

Thanks! Let's merge after tests pass.

@jasongrout
Copy link
Contributor

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
Copy link
Member Author

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 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

Maybe npmjs.org is having a hiccup?

@jasongrout
Copy link
Contributor

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
@vidartf
Copy link
Member

vidartf commented May 4, 2019

Will the IModelDB things affect the current RTC work?

@cupdike
Copy link

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

@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

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

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

@blink1073
Copy link
Member

👍

@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Aug 7, 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.
Labels
bug pkg:outputarea status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keras progress bars cause jupyterlab to freeze firefox and chrome
5 participants