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

Shrink ComputedValue using a bitfield #3880

Merged
merged 1 commit into from
Jun 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/dirty-turtles-marry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"mobx": patch
---

Shrink ComputedValue using a bitfield
38 changes: 19 additions & 19 deletions packages/mobx/__tests__/v5/base/errorhandling.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const utils = require("../../v5/utils/test-utils")

const { observable, computed, $mobx, autorun } = mobx

const voidObserver = function () { }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't change this on purpose, I think the formatter is just getting excited because I changed other things in this file

const voidObserver = function () {}

function checkGlobalState() {
const gs = mobx._getGlobalState()
Expand Down Expand Up @@ -148,7 +148,7 @@ test("deny state changes in views", function () {

m.reaction(
() => z.get(),
() => { }
() => {}
)
expect(
utils.grabConsole(() => {
Expand Down Expand Up @@ -194,7 +194,7 @@ test("deny array change in view", function (done) {
}).not.toThrow()
m.reaction(
() => z.length,
() => { }
() => {}
)

expect(
Expand Down Expand Up @@ -483,7 +483,7 @@ test("peeking inside erroring computed value doesn't bork (global) state", () =>
b.get()
}).toThrowError(/chocolademelk/)

expect(a.isPendingUnobservation_).toBe(false)
expect(a.isPendingUnobservation).toBe(false)
expect(a.observers_.size).toBe(0)
expect(a.diffValue_).toBe(0)
expect(a.lowestObserverState_).toBe(-1)
Expand All @@ -493,15 +493,15 @@ test("peeking inside erroring computed value doesn't bork (global) state", () =>
expect(b.dependenciesState_).toBe(-1) // NOT_TRACKING
expect(b.observing_.length).toBe(0)
expect(b.newObserving_).toBe(null)
expect(b.isPendingUnobservation_).toBe(false)
expect(b.isPendingUnobservation).toBe(false)
expect(b.observers_.size).toBe(0)
expect(b.diffValue_).toBe(0)
expect(b.lowestObserverState_).toBe(0)
expect(b.unboundDepsCount_).toBe(0)
expect(() => {
b.get()
}).toThrowError(/chocolademelk/)
expect(b.isComputing_).toBe(false)
expect(b.isComputing).toBe(false)

checkGlobalState()
})
Expand All @@ -521,7 +521,7 @@ describe("peeking inside autorun doesn't bork (global) state", () => {
expect(r).toBe(1)

test("it should update correctly initially", () => {
expect(a.isPendingUnobservation_).toBe(false)
expect(a.isPendingUnobservation).toBe(false)
expect(a.observers_.size).toBe(1)
expect(a.diffValue_).toBe(0)
expect(a.lowestObserverState_).toBe(-1)
Expand All @@ -531,13 +531,13 @@ describe("peeking inside autorun doesn't bork (global) state", () => {
expect(b.dependenciesState_).toBe(0)
expect(b.observing_.length).toBe(1)
expect(b.newObserving_).toBe(null)
expect(b.isPendingUnobservation_).toBe(false)
expect(b.isPendingUnobservation).toBe(false)
expect(b.observers_.size).toBe(1)
expect(b.diffValue_).toBe(0)
expect(b.lowestObserverState_).toBe(0)
expect(b.unboundDepsCount_).toBe(1) // value is always the last bound amount of observers
expect(b.value_).toBe(1)
expect(b.isComputing_).toBe(false)
expect(b.isComputing).toBe(false)

expect(c.dependenciesState_).toBe(0)
expect(c.observing_.length).toBe(1)
Expand All @@ -558,7 +558,7 @@ describe("peeking inside autorun doesn't bork (global) state", () => {
}, /chocolademelk/)
expect(r).toBe(2)

expect(a.isPendingUnobservation_).toBe(false)
expect(a.isPendingUnobservation).toBe(false)
expect(a.observers_.size).toBe(1)
expect(a.diffValue_).toBe(0)
expect(a.lowestObserverState_).toBe(0)
Expand All @@ -568,12 +568,12 @@ describe("peeking inside autorun doesn't bork (global) state", () => {
expect(b.dependenciesState_).toBe(0) // up to date (for what it's worth)
expect(b.observing_.length).toBe(1)
expect(b.newObserving_).toBe(null)
expect(b.isPendingUnobservation_).toBe(false)
expect(b.isPendingUnobservation).toBe(false)
expect(b.observers_.size).toBe(1)
expect(b.diffValue_).toBe(0)
expect(b.lowestObserverState_).toBe(0)
expect(b.unboundDepsCount_).toBe(1)
expect(b.isComputing_).toBe(false)
expect(b.isComputing).toBe(false)
expect(() => b.get()).toThrowError(/chocolademelk/)

expect(c.dependenciesState_).toBe(0)
Expand All @@ -594,7 +594,7 @@ describe("peeking inside autorun doesn't bork (global) state", () => {
a.set(3)
expect(r).toBe(3)

expect(a.isPendingUnobservation_).toBe(false)
expect(a.isPendingUnobservation).toBe(false)
expect(a.observers_.size).toBe(1)
expect(a.diffValue_).toBe(0)
expect(a.lowestObserverState_).toBe(0)
Expand All @@ -604,13 +604,13 @@ describe("peeking inside autorun doesn't bork (global) state", () => {
expect(b.dependenciesState_).toBe(0) // up to date
expect(b.observing_.length).toBe(1)
expect(b.newObserving_).toBe(null)
expect(b.isPendingUnobservation_).toBe(false)
expect(b.isPendingUnobservation).toBe(false)
expect(b.observers_.size).toBe(1)
expect(b.diffValue_).toBe(0)
expect(b.lowestObserverState_).toBe(0)
expect(b.unboundDepsCount_).toBe(1)
expect(b.value_).toBe(3)
expect(b.isComputing_).toBe(false)
expect(b.isComputing).toBe(false)

expect(c.dependenciesState_).toBe(0)
expect(c.observing_.length).toBe(1)
Expand All @@ -628,7 +628,7 @@ describe("peeking inside autorun doesn't bork (global) state", () => {
test("it should clean up correctly", () => {
d()

expect(a.isPendingUnobservation_).toBe(false)
expect(a.isPendingUnobservation).toBe(false)
expect(a.observers_.size).toBe(0)
expect(a.diffValue_).toBe(0)
expect(a.lowestObserverState_).toBe(0)
Expand All @@ -638,13 +638,13 @@ describe("peeking inside autorun doesn't bork (global) state", () => {
expect(b.dependenciesState_).toBe(-1) // not tracking
expect(b.observing_.length).toBe(0)
expect(b.newObserving_).toBe(null)
expect(b.isPendingUnobservation_).toBe(false)
expect(b.isPendingUnobservation).toBe(false)
expect(b.observers_.size).toBe(0)
expect(b.diffValue_).toBe(0)
expect(b.lowestObserverState_).toBe(0)
expect(b.unboundDepsCount_).toBe(1)
expect(b.value_).not.toBe(3)
expect(b.isComputing_).toBe(false)
expect(b.isComputing).toBe(false)

expect(c.dependenciesState_).toBe(-1)
expect(c.observing_.length).toBe(0)
Expand Down Expand Up @@ -866,4 +866,4 @@ describe("es5 compat warnings", () => {
})
})

test("should throw when adding properties in ES5 compat mode", () => { })
test("should throw when adding properties in ES5 compat mode", () => {})
4 changes: 2 additions & 2 deletions packages/mobx/src/core/atom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ export interface IAtom extends IObservable {
}

export class Atom implements IAtom {
isPendingUnobservation_ = false // for effective unobserving. BaseAtom has true, for extra optimization, so its onBecomeUnobserved never gets called, because it's not needed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed these so that they are no longer mangled (don't end with _). This is so that they still match the new getter/setter names when accessed. Unfortunately the code generated in the final bundle for getters/setters has their name in quotes e.g. "isPendingUnobservation" and name mangling is not smart enough to figure out these are being used as property names.

isBeingObserved_ = false
isPendingUnobservation = false // for effective unobserving. BaseAtom has true, for extra optimization, so its onBecomeUnobserved never gets called, because it's not needed
isBeingObserved = false
observers_ = new Set<IDerivation>()

diffValue_ = 0
Expand Down
64 changes: 54 additions & 10 deletions packages/mobx/src/core/computedvalue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,18 @@ export type IComputedDidChange<T = any> = {
oldValue: T | undefined
}

function getFlag(flags: number, mask: number) {
return !!(flags & mask)
}
function setFlag(flags: number, mask: number, newValue: boolean): number {
if (newValue) {
flags |= mask
} else {
flags &= ~mask
}
return flags
}

/**
* A node in the state dependency root that observes other nodes, and can be observed itself.
*
Expand All @@ -79,8 +91,6 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
dependenciesState_ = IDerivationState_.NOT_TRACKING_
observing_: IObservable[] = [] // nodes we are looking at. Our value depends on these nodes
newObserving_ = null // during tracking it's an array with new observed observers
isBeingObserved_ = false
isPendingUnobservation_: boolean = false
observers_ = new Set<IDerivation>()
diffValue_ = 0
runId_ = 0
Expand All @@ -90,8 +100,13 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
protected value_: T | undefined | CaughtException = new CaughtException(null)
name_: string
triggeredBy_?: string
isComputing_: boolean = false // to check for cycles
isRunningSetter_: boolean = false

private static readonly isComputingMask_ = 0b0001
private static readonly isRunningSetterMask_ = 0b0010
private static readonly isBeingObservedMask_ = 0b0100
private static readonly isPendingUnobservationMask_ = 0b1000
private flags_ = 0b0000

derivation: () => T // N.B: unminified as it is used by MST
setter_?: (value: T) => void
isTracing_: TraceMode = TraceMode.NONE
Expand Down Expand Up @@ -153,12 +168,41 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
}
}

// to check for cycles
private get isComputing(): boolean {
return getFlag(this.flags_, ComputedValue.isComputingMask_)
}
private set isComputing(newValue: boolean) {
this.flags_ = setFlag(this.flags_, ComputedValue.isComputingMask_, newValue)
}

private get isRunningSetter(): boolean {
return getFlag(this.flags_, ComputedValue.isRunningSetterMask_)
}
private set isRunningSetter(newValue: boolean) {
this.flags_ = setFlag(this.flags_, ComputedValue.isRunningSetterMask_, newValue)
}

get isBeingObserved(): boolean {
return getFlag(this.flags_, ComputedValue.isBeingObservedMask_)
}
set isBeingObserved(newValue: boolean) {
this.flags_ = setFlag(this.flags_, ComputedValue.isBeingObservedMask_, newValue)
}

get isPendingUnobservation(): boolean {
return getFlag(this.flags_, ComputedValue.isPendingUnobservationMask_)
}
set isPendingUnobservation(newValue: boolean) {
this.flags_ = setFlag(this.flags_, ComputedValue.isPendingUnobservationMask_, newValue)
}

/**
* Returns the current value of this computed value.
* Will evaluate its computation first if needed.
*/
public get(): T {
if (this.isComputing_) {
if (this.isComputing) {
die(32, this.name_, this.derivation)
}
if (
Expand Down Expand Up @@ -196,14 +240,14 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva

public set(value: T) {
if (this.setter_) {
if (this.isRunningSetter_) {
if (this.isRunningSetter) {
die(33, this.name_)
}
this.isRunningSetter_ = true
this.isRunningSetter = true
try {
this.setter_.call(this.scope_, value)
} finally {
this.isRunningSetter_ = false
this.isRunningSetter = false
}
} else {
die(34, this.name_)
Expand Down Expand Up @@ -242,7 +286,7 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
}

computeValue_(track: boolean) {
this.isComputing_ = true
this.isComputing = true
// don't allow state changes during computation
const prev = allowStateChangesStart(false)
let res: T | CaughtException
Expand All @@ -260,7 +304,7 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
}
}
allowStateChangesEnd(prev)
this.isComputing_ = false
this.isComputing = false
return res
}

Expand Down
20 changes: 10 additions & 10 deletions packages/mobx/src/core/observable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ export interface IObservable extends IDepTreeNode {
* the dependency is already established
*/
lastAccessedBy_: number
isBeingObserved_: boolean
isBeingObserved: boolean

lowestObserverState_: IDerivationState_ // Used to avoid redundant propagations
isPendingUnobservation_: boolean // Used to push itself to global.pendingUnobservations at most once per batch.
isPendingUnobservation: boolean // Used to push itself to global.pendingUnobservations at most once per batch.

observers_: Set<IDerivation>

Expand Down Expand Up @@ -91,9 +91,9 @@ export function removeObserver(observable: IObservable, node: IDerivation) {
}

export function queueForUnobservation(observable: IObservable) {
if (observable.isPendingUnobservation_ === false) {
if (observable.isPendingUnobservation === false) {
// invariant(observable._observers.length === 0, "INTERNAL ERROR, should only queue for unobservation unobserved observables");
observable.isPendingUnobservation_ = true
observable.isPendingUnobservation = true
globalState.pendingUnobservations.push(observable)
}
}
Expand All @@ -114,11 +114,11 @@ export function endBatch() {
const list = globalState.pendingUnobservations
for (let i = 0; i < list.length; i++) {
const observable = list[i]
observable.isPendingUnobservation_ = false
observable.isPendingUnobservation = false
if (observable.observers_.size === 0) {
if (observable.isBeingObserved_) {
if (observable.isBeingObserved) {
// if this observable had reactive observers, trigger the hooks
observable.isBeingObserved_ = false
observable.isBeingObserved = false
observable.onBUO()
}
if (observable instanceof ComputedValue) {
Expand Down Expand Up @@ -146,12 +146,12 @@ export function reportObserved(observable: IObservable): boolean {
observable.lastAccessedBy_ = derivation.runId_
// Tried storing newObserving, or observing, or both as Set, but performance didn't come close...
derivation.newObserving_![derivation.unboundDepsCount_++] = observable
if (!observable.isBeingObserved_ && globalState.trackingContext) {
observable.isBeingObserved_ = true
if (!observable.isBeingObserved && globalState.trackingContext) {
observable.isBeingObserved = true
observable.onBO()
}
}
return observable.isBeingObserved_
return observable.isBeingObserved
} else if (observable.observers_.size === 0 && globalState.inBatch > 0) {
queueForUnobservation(observable)
}
Expand Down