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

Spring Cleaning, and Load speedup #4515

Merged
merged 6 commits into from Nov 13, 2013
Merged

Conversation

Carreau
Copy link
Member

@Carreau Carreau commented Nov 8, 2013

  1. prompt '*' strore fix + tab remove tooltip
    tab was not cancelling tooltip bringing to cases where you could have
    tooltip andcompleter open.

Do not store '*' when serializing cells.

get rid of most slowdown at notebook loading.

  1. Do not setOption('mode',new_mode) on CM if new and old mode are the
    same. It triggert a lot of calculation of bounding box in the
    end.

  2. Do not select cell when loading the notebook it triggers
    a lot of CM even that check visible things and so on and so
    forth. So add a option to add_cell_at_index not to select it

  3. jQuery $.attr has some magics, but has a slight overhead on
    real native ELEM.setAttribute DOM method. Seem slight improvement
    when loads of PNGs on one page

@minrk
Copy link
Member

minrk commented Nov 8, 2013

This is great. I saw the set mode slowdown when profiling a many-cell load. That and CM instantiation are the main issues loading large notebooks.

@minrk
Copy link
Member

minrk commented Nov 8, 2013

JS tests look like real failure.

@@ -789,8 +789,8 @@ var IPython = (function (IPython) {
*
* @return cell {cell|null} created cell or null
**/
Notebook.prototype.insert_cell_at_index = function(type, index){

Notebook.prototype.insert_cell_at_index = function(type, index, opts){
Copy link
Member

Choose a reason for hiding this comment

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

In my dual mode editor branch #3605 I completely rework the cell selection logic and fix this as well - although in a different manner. Can we leave it out of this PR as it will make that PR more difficult to rebase.

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, will do.

@Carreau
Copy link
Member Author

Carreau commented Nov 8, 2013

This is great. I saw the set mode slowdown when profiling a many-cell load. That and CM instantiation are the main issues loading large notebooks.

I'm not sure it's instantiation, but the combinaison (instantiation+set_text) that triggers 2 redraw. I'm wondering if we can directly create a sell from JSON, instead of creating and then fromJSON.

@minrk
Copy link
Member

minrk commented Nov 8, 2013

Removing the duplicate set_mode is definitely a great improvement, and buys us some time. I think with Brian's modal change, we can consider lazy instantiation, where we don't instantiate CM until the cell is focused for the first time.

@Carreau
Copy link
Member Author

Carreau commented Nov 9, 2013

where we don't instantiate CM until the cell is focused for the first time.

And how would you show code for code cell ? , use a 1pass highlight ? it will not be faster.

@ellisonbg I removed the part you asked me to remove, but it is one of the major cause of
slow load time.
BTW, you haven't pushed on #3605 in month; would yo like to update with the new things you have ?

@ghost ghost assigned Carreau Nov 13, 2013
tab was not cancelling tooltip bringing to cases where you could have
tooltip andcompleter open.

Do not store '*' when serializing cells.
@Carreau
Copy link
Member Author

Carreau commented Nov 13, 2013

Rebased on master for conflict.

1) Do not setOption('mode',new_mode) on CM if new and old mode are the
   same. It triggert **a lot** of calculation of bounding box in the
   end.

2) Do **not** select cell when loading the notebook it triggers
   **a lot** of CM even that check visible things and so on and so
   forth. So add a option to add_cell_at_index not to select it

3) jQuery $.attr has some magics, but has a slight overhead on
   real native ELEM.setAttribute DOM method. Seem slight improvement
   when loads of PNGs on one page
optimisation are not obvious, but order and time of attribute creation
in javascript have impact for VMs apparently (Google IO talk on V8)
do not add the option not to select each cell in a row when buildogn the
notebook at load time.
@@ -576,12 +576,13 @@ var IPython = (function (IPython) {

OutputArea.prototype.append_png = function (png, md, element) {
var toinsert = this.create_output_subarea(md, "output_png");
var img = $("<img/>").attr('src','data:image/png;base64,'+png);
var img = $("<img/>")
Copy link
Member

Choose a reason for hiding this comment

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

semicolon

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

@minrk
Copy link
Member

minrk commented Nov 13, 2013

Looks good. I'll try to play with it and merge today.

@minrk
Copy link
Member

minrk commented Nov 13, 2013

This is working smoothly. Definitely better, but still plenty of room for improvement. Merging.

minrk added a commit that referenced this pull request Nov 13, 2013
Spring Cleaning, and  Load speedup

0) prompt '*' strore fix + tab remove tooltip
tab was not cancelling tooltip bringing to cases where you could have
tooltip andcompleter open.

Do not store '*' when serializing cells.

get rid of most slowdown at notebook loading.
1) Do not setOption('mode',new_mode) on CM if new and old mode are the
same. It triggert a lot of calculation of bounding box in the
end.

2) Do not select cell when loading the notebook it triggers
a lot of CM even that check visible things and so on and so
forth. So add a option to add_cell_at_index not to select it

3) jQuery $.attr has some magics, but has a slight overhead on
real native ELEM.setAttribute DOM method. Seem slight improvement
when loads of PNGs on one page
@minrk minrk merged commit af80839 into ipython:master Nov 13, 2013
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Spring Cleaning, and  Load speedup

0) prompt '*' strore fix + tab remove tooltip
tab was not cancelling tooltip bringing to cases where you could have
tooltip andcompleter open.

Do not store '*' when serializing cells.

get rid of most slowdown at notebook loading.
1) Do not setOption('mode',new_mode) on CM if new and old mode are the
same. It triggert a lot of calculation of bounding box in the
end.

2) Do not select cell when loading the notebook it triggers
a lot of CM even that check visible things and so on and so
forth. So add a option to add_cell_at_index not to select it

3) jQuery $.attr has some magics, but has a slight overhead on
real native ELEM.setAttribute DOM method. Seem slight improvement
when loads of PNGs on one page
@Carreau Carreau deleted the load-speedup branch December 15, 2014 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants