Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Closing a tab towards the beginning of a tab strip with many tabs has performance issue #14081

Closed
petemill opened this issue May 10, 2018 · 2 comments

Comments

@petemill
Copy link
Member

petemill commented May 10, 2018

Description

When removing a tab from state due to a tab close, we delete it from an Immutable.List. This causes every tab with a higher index in that object to have a new key. When we send the updated state to each window, we first compute the diff, which results in a new change operation for every single property of every single tab, since the key for the tab state is the index of the item in the List. This can block the thread for up to 1 second. Then this huge array is sent over IPC to each window, which then runs through the array and 'patches' the in-memory appState for that window, which can take 1+ seconds, locking up the UI thread. If closing multiple tabs at the same time, this could lock up the thread even more.

Test plan / Steps to Reproduce

  1. Open 2, 3 or 4 windows with 150 tabs in each
  2. Close a tab towards the beginning of the tab strip

Actual result:
on macOS: beach ball for a few seconds, relative to the number of tabs and windows open

Expected result:
tab is closed quickly

Reproduces how often:
100%

Brave Version

about:brave info:
0.22.703

Reproducible on current live release:
Probably, as this code path is unchanged

Additional Information

Simplest fix is to not delete an item from the tabstate unless it is the last index. Instead, we could set it to null or an empty Immutable.Map. This would avoid a huge diff when deleting an entry.

@petemill petemill added this to the 0.22.x Release 3 (Beta channel) milestone May 10, 2018
@petemill petemill self-assigned this May 10, 2018
petemill added a commit that referenced this issue May 10, 2018
Fix #14081
Avoids having to send large diffs covering every property of every tab with an index higher than the one being removed. The browser diff and window patch operations are expensive.
@petemill
Copy link
Member Author

Should be fixed in next beta build

petemill added a commit that referenced this issue May 10, 2018
Fix #14081
Avoids having to send large diffs covering every property of every tab with an index higher than the one being removed. The browser diff and window patch operations are expensive.
petemill added a commit that referenced this issue May 10, 2018
Fix #14081
Avoids having to send large diffs covering every property of every tab with an index higher than the one being removed. The browser diff and window patch operations are expensive.
@LaurenWags
Copy link
Member

LaurenWags commented May 10, 2018

Verified with macOS 10.12.6 using

  • 0.22.706 e11b027
  • muon 6.0.9
  • libchromiumcontent 66.0.3359.139

Verified on Windows x64

  • 0.22.706 e11b027
  • libchromiumcontent 66.0.3359.139
  • muon: 6.0.9

Verified on Ubuntu 17.10 x64

  • 0.22.706 e11b027
  • libchromiumcontent 66.0.3359.139
  • muon: 6.0.9

petemill added a commit that referenced this issue May 11, 2018
Fix #14081
Avoids having to send large diffs covering every property of every tab with an index higher than the one being removed. The browser diff and window patch operations are expensive.
petemill added a commit that referenced this issue May 11, 2018
Fix #14081
Avoids having to send large diffs covering every property of every tab with an index higher than the one being removed. The browser diff and window patch operations are expensive.
petemill added a commit that referenced this issue May 11, 2018
Fix #14081
Avoids having to send large diffs covering every property of every tab with an index higher than the one being removed. The browser diff and window patch operations are expensive.
petemill added a commit that referenced this issue May 11, 2018
Fix #14081
Avoids having to send large diffs covering every property of every tab with an index higher than the one being removed. The browser diff and window patch operations are expensive.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.