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

Stop beach balling with this one simple trick #10305

Merged
merged 1 commit into from
Aug 9, 2017
Merged

Stop beach balling with this one simple trick #10305

merged 1 commit into from
Aug 9, 2017

Conversation

bbondy
Copy link
Member

@bbondy bbondy commented Aug 4, 2017

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

@bbondy bbondy added this to the 0.18.x Hotfix milestone Aug 4, 2017
@bbondy bbondy changed the title WIP: Stop beach balling with this easy trick WIP: Stop beach balling with this one simple trick Aug 5, 2017
@bsclifton bsclifton requested review from bridiver, diracdeltas and bsclifton and removed request for bridiver, diracdeltas and bsclifton August 7, 2017 06:31
@bsclifton
Copy link
Member

bsclifton commented Aug 7, 2017

REVIEWERS HATE HIM

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

@cezaraugusto
Copy link
Contributor

cezaraugusto commented Aug 8, 2017

screen shot 2017-08-07 at 11 44 18 pm

screen shot 2017-08-08 at 10 27 32 pm

screen shot 2017-08-07 at 11 54 21 pm

@bbondy bbondy force-pushed the beach-bawler branch 8 times, most recently from 0ff3668 to 225dd54 Compare August 9, 2017 01:58
@bbondy bbondy changed the title WIP: Stop beach balling with this one simple trick Stop beach balling with this one simple trick Aug 9, 2017
Fix #10094

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

Instead we just keep everything in Immutable
@bbondy
Copy link
Member Author

bbondy commented Aug 9, 2017

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

@@ -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())
Copy link
Collaborator

Choose a reason for hiding this comment

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

why the change from delete to an empty list?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

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

++

@luixxiul luixxiul added the perf label Aug 9, 2017
@luixxiul
Copy link
Contributor

luixxiul commented Aug 9, 2017

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 bbondy merged commit c739ff2 into master Aug 9, 2017
@bbondy
Copy link
Member Author

bbondy commented Aug 9, 2017

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.

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})
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

👌

})
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})
Copy link
Contributor

Choose a reason for hiding this comment

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

watch out master ken

bbondy added a commit that referenced this pull request Aug 9, 2017
Stop beach balling with this one simple trick
@bsclifton bsclifton deleted the beach-bawler branch August 9, 2017 05:21
bbondy added a commit that referenced this pull request Aug 9, 2017
Stop beach balling with this one simple trick
bbondy added a commit that referenced this pull request Aug 9, 2017
Stop beach balling with this one simple trick
@bbondy
Copy link
Member Author

bbondy commented Aug 9, 2017

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
Copy link
Contributor

luixxiul commented Aug 9, 2017

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

@cndouglas
Copy link

Bug possibly caused by this: #10371

@cndouglas
Copy link

Also, #10376

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants