[Immutable.js experiment] Memory tool

RESOLVED WONTFIX

Status

DevTools
Shared Components
P2
normal
RESOLVED WONTFIX
2 years ago
a month ago

People

(Reporter: linclark, Assigned: linclark)

Tracking

Trunk
Dependency tree / graph

Firefox Tracking Flags

(firefox47 affected)

Details

(Whiteboard: [btpp-fix-later])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
The memory tool currently fakes immutability.

https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/reducers/diffing.js#108

Let's see what this would look like if handled with Immutable.js
(Assignee)

Updated

2 years ago
Blocks: 1253375
(Assignee)

Updated

2 years ago
Assignee: nobody → lclark
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Depends on: 1253784
(Assignee)

Comment 1

2 years ago
Created attachment 8727014 [details] [diff] [review]
Bug1253376-immutable-memory-tool.patch

This is a quick and dirty first pass at the diffing state.

A couple of things are temporarily broken/inconsistent:

- You'll need to apply the patch from Bug 1253784 first.
- (Temporarily) I'm breaking snapshot census display. This is because I changed the census object to Immutable in the diffing reducer, but not the snapshot. If you continue with a diff, you'll see that it works.
- (Temporarily) Some of the deeper nested objects are still mutable.

For discussion:

- How do folks feel about having to use .get() (in components, etc)? I know some teams are converting to regular objects in selectors via reselect.
- Do we want to make the whole state tree immutable? If we do, we'll need to use something like redux-immutable.
- Are there any other questions we want to answer before moving forward with Immutable?
I'd be interested to see that with Immutable.Record [1] in place of Immutable.Map. My guess is that the patch would be considerably smaller, and more natural.

The downside of using Record is the need to define your records, but if we were to keep all of our Record definitions in the same place then there would be a nice self documentation effect.

[1]: https://facebook.github.io/immutable-js/docs/#/Record
(Assignee)

Comment 3

2 years ago
Created attachment 8728373 [details] [diff] [review]
Bug1253376-immutable-memory-tool.patch

You're right, that is waaaaay smaller and more natural. It should also remove any need for Redux Immutable, I think.

New patch, but it's still quick and dirty so previous provisos apply.
Attachment #8727014 - Attachment is obsolete: true
(Assignee)

Comment 4

2 years ago
Created attachment 8728672 [details] [diff] [review]
Bug1253376-immutable-memory-tool.patch

This patch uses setIn(), deleteIn(), etc to simplify the logic.
Attachment #8728373 - Attachment is obsolete: true
A couple things:

* The memory tool doesn't "fake" immutability, for the most part. Almost all state data is actually immutable, as it is Object.freeze()'d and updates happen via `immutableUpdate`. The exception is mostly data received directly via structured cloning from the HeapAnalysisWorker rather than generated by the frontend itself when reducing actions. I don't think it makes sense to walk this data and convert it to Immutable structures or use Immutable.js in the worker.

* I'm wary of going all-in on Immutable.js for all-the-things right off the bat. Such a large scale change calls for closer evaluation, and I just haven't done that yet. I remain tentatively optimistic, however.

* A great place to start would be with immutable sets, which Immutable.js has as (you guessed it) `Immutable.Set`. Right now, we mutate native `Set` objects, but update a wrapper object when we do so that we can maintain referential transparency. This is the closest we come to faking immutability. This would be a great piece of cruft to remove and replace with a proper immutable set. https://dxr.mozilla.org/mozilla-central/search?q=%22warning%3A+mutable+operations%22+path%3Adevtools%2Fclient%2Fmemory%2Freducers&redirect=true&case=false
Err I meant "maintain referential *equality*", and specifically inside `shouldComponentUpdate` component methods.
(Assignee)

Comment 7

2 years ago
Right, if you look in the patch you'll see that it does use Immutable.Set, very specifically in that great place to start that you mention.
(Assignee)

Updated

2 years ago
Priority: -- → P2
Whiteboard: [btpp-fix-later]
(Assignee)

Comment 9

2 years ago
I added a patch replace the cruft that Nick referred to in Comment 5. See Bug 1253784
See Also: → bug 1239436
(Assignee)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.