Stop beach balling with this one simple trick #10305

bbondy · 2017-08-04T23:58:36Z

Fix #10094

The problem is with toJS being called to convert the immutable app state

Instead we just keep everything in Immutable

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Test Plan:

Please fully test session restore
- active tab is preserved
- windows with tabs are preserved
- test that updates do not clear session storage
- etc.
Note that while testing this I noticed Crashing main process is losing window state #10349 but it is not related to this task. It happens even on production.

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

bsclifton · 2017-08-07T06:32:58Z

REVIEWERS HATE HIM

Whoops- just noticed this is still a WIP; sorry for assigning folks (I had just picked the recommended reviewers 😄 )

cezaraugusto · 2017-08-08T04:17:07Z

Fix #10094 The problem is with toJS being called to convert the immutable app state Instead we just keep everything in Immutable

bbondy · 2017-08-09T02:39:49Z

before:

cleanAppData: 3159.975ms
getStoragePath: 0.033ms
stringify: 125.966ms
writeImportantBlocking: 83.684ms
writeImportant: 103.235ms
resolve: 0.008ms

after:

cleanAppData: 0.904ms
getStoragePath: 0.018ms
stringify: 261.541ms
writeImportantBlocking: 57.554ms
writeImportant: 75.337ms
resolve: 0.008ms

bridiver · 2017-08-09T02:41:40Z

app/common/state/tabState.js

@@ -610,7 +610,7 @@ const tabState = {
    state = tabState.removeTabField(state, 'messageBoxDetail')
    state = tabState.removeTabField(state, 'frame')
    state = state.delete('tabsInternal')
-    return state.delete('tabs')
+    return state.set('tabs', Immutable.List())


why the change from delete to an empty list?

because we used to set default app state in 2 places but now just one place. Via Object.assign after a clean, but I'd of had to have 2 different methods for that, one for immutable and one for not.

I think that might be a problem because at some point in the near future we won't be deleting the tabs and so this won't be equivalent to a default state

that would be perfectly fine to not delete the state. It's just that if you put undefined for 'tabs' in state here then it won't get filled later.

bridiver

++

luixxiul · 2017-08-09T03:14:37Z

Looking forward to see this landed :-) We will have to search other issues which can be closed thanks to this, ones labelled with perf. I'm going to do QA on #10094 with my own browser's profile which cannot be shared.

bbondy · 2017-08-09T03:51:43Z

merged to master, but this is going to take me a while to rebase for other branches.
I'll update here as I do other branches.

luixxiul · 2017-08-09T03:51:45Z

test/unit/app/common/state/immutableUtilTest.js

+      assert.deepEqual(immutableUtil.deleteImmutablePaths(data, ['a', 'b']).toJS(), {c: 3})
+    })
+    it('removes properties with array string paths', function () {
+      const data = Immutable.fromJS({a: {a1: 4, a2: 8}, c: 'Cezar learnt directly from master ken', d: 5})


@cezaraugusto :D

cezaraugusto · 2017-08-09T04:24:09Z

test/unit/app/common/state/immutableUtilTest.js

+  })
+  describe('deleteImmutablePaths', function () {
+    it('removes properties with simple strings', function () {
+      const data = Immutable.fromJS({a: 'Cezar is a black belt in ameri-do-te', b: 2, c: 3})


watch out master ken

Stop beach balling with this one simple trick

bbondy · 2017-08-09T05:33:08Z

Merged to 0.20.x, 0.19.x, and 0.18.x.
Big rebase on 0.20x, other 2 were cleanly applied from the 0.20.x rebase.

luixxiul · 2017-08-09T05:43:57Z

@bbondy should we do QA on each release until 0.21 to see if the rebase was successful?

cndouglas · 2017-08-09T18:05:56Z

Bug possibly caused by this: #10371

cndouglas · 2017-08-09T21:39:33Z

Also, #10376

bbondy added the PR/work-in-progress ⚒ label Aug 4, 2017

bbondy added this to the 0.18.x Hotfix milestone Aug 4, 2017

bbondy changed the title ~~WIP: Stop beach balling with this easy trick~~ WIP: Stop beach balling with this one simple trick Aug 5, 2017

bbondy force-pushed the beach-bawler branch from d616cc8 to fc814d8 Compare August 5, 2017 03:47

luixxiul assigned bbondy Aug 6, 2017

bsclifton requested review from bridiver, diracdeltas and bsclifton and removed request for bridiver, diracdeltas and bsclifton August 7, 2017 06:31

luixxiul mentioned this pull request Aug 8, 2017

Browser freezes per 5 minutes with existing profile #10094

Closed

bbondy force-pushed the beach-bawler branch 8 times, most recently from 0ff3668 to 225dd54 Compare August 9, 2017 01:58

bbondy changed the title ~~WIP: Stop beach balling with this one simple trick~~ Stop beach balling with this one simple trick Aug 9, 2017

Stop beach balling with this one simple trick

e3d1dd2

Fix #10094 The problem is with toJS being called to convert the immutable app state Instead we just keep everything in Immutable

bbondy force-pushed the beach-bawler branch from 225dd54 to e3d1dd2 Compare August 9, 2017 02:06

bbondy requested review from bridiver and bsclifton August 9, 2017 02:27

bbondy removed the PR/work-in-progress ⚒ label Aug 9, 2017

bridiver reviewed Aug 9, 2017

View reviewed changes

bridiver approved these changes Aug 9, 2017

View reviewed changes

luixxiul added the perf label Aug 9, 2017

bbondy merged commit c739ff2 into master Aug 9, 2017

luixxiul reviewed Aug 9, 2017

View reviewed changes

cezaraugusto reviewed Aug 9, 2017

View reviewed changes

bbondy added a commit that referenced this pull request Aug 9, 2017

Merge pull request #10305 from brave/beach-bawler

c8f0738

Stop beach balling with this one simple trick

bsclifton deleted the beach-bawler branch August 9, 2017 05:21

bbondy added a commit that referenced this pull request Aug 9, 2017

Merge pull request #10305 from brave/beach-bawler

d21ca60

Stop beach balling with this one simple trick

bbondy added a commit that referenced this pull request Aug 9, 2017

Merge pull request #10305 from brave/beach-bawler

cccb3dd

Stop beach balling with this one simple trick

cndouglas mentioned this pull request Aug 9, 2017

0.18.20: Cannot create a new private/session tab without an active window #10371

Closed

NejcZdovc mentioned this pull request Aug 19, 2017

Session runPreMigrations is not working #10595

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop beach balling with this one simple trick #10305

Stop beach balling with this one simple trick #10305

bbondy commented Aug 4, 2017 •

edited

Loading

bsclifton commented Aug 7, 2017 •

edited

Loading

cezaraugusto commented Aug 8, 2017 •

edited by bbondy

Loading

bbondy commented Aug 9, 2017

bridiver Aug 9, 2017

bbondy Aug 9, 2017

bridiver Aug 9, 2017

bbondy Aug 9, 2017

bridiver left a comment

luixxiul commented Aug 9, 2017

bbondy commented Aug 9, 2017

luixxiul Aug 9, 2017

cezaraugusto Aug 9, 2017

cezaraugusto Aug 9, 2017

bbondy commented Aug 9, 2017

luixxiul commented Aug 9, 2017

cndouglas commented Aug 9, 2017

cndouglas commented Aug 9, 2017

Stop beach balling with this one simple trick #10305

Stop beach balling with this one simple trick #10305

Conversation

bbondy commented Aug 4, 2017 • edited Loading

bsclifton commented Aug 7, 2017 • edited Loading

cezaraugusto commented Aug 8, 2017 • edited by bbondy Loading

bbondy commented Aug 9, 2017

bridiver Aug 9, 2017

Choose a reason for hiding this comment

bbondy Aug 9, 2017

Choose a reason for hiding this comment

bridiver Aug 9, 2017

Choose a reason for hiding this comment

bbondy Aug 9, 2017

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

luixxiul commented Aug 9, 2017

bbondy commented Aug 9, 2017

luixxiul Aug 9, 2017

Choose a reason for hiding this comment

cezaraugusto Aug 9, 2017

Choose a reason for hiding this comment

cezaraugusto Aug 9, 2017

Choose a reason for hiding this comment

bbondy commented Aug 9, 2017

luixxiul commented Aug 9, 2017

cndouglas commented Aug 9, 2017

cndouglas commented Aug 9, 2017

bbondy commented Aug 4, 2017 •

edited

Loading

bsclifton commented Aug 7, 2017 •

edited

Loading

cezaraugusto commented Aug 8, 2017 •

edited by bbondy

Loading