Use union of strings for Cache in core

[snnsnn]

Using union of strings for Cache type inside the core package improves clarity and code readability. Clearly you did not want to use an enum to avoid property lookup. This solution even removes the variable lookup. There is performance gain but not that significant.

type Cache =
  | 'CLEAN'
  | 'CHECK'
  | 'DIRTY';

type CacheWithoutClean = Exclude<Cache, 'CLEAN'>;

Please note that we still get type checking even though we use string expressions. For example, you will get type error for the following function:

const checkIfDirty = (state: CacheState) => {
  return state !== 'CLEAX';
}

Code is also more readable due to more dicernible colors.

if (this._value !== value && this.observers) {
  for (let i = 0; i < this.observers.length; i++) {
    this.observers[i].stale('DIRTY');
  }
}

It also eliminates possible future bugs emanating from zero being a falsy value, if implementation evolves into one that use the boolean logic over state values.

I did some benchmarks using different bench-marking tools and platforms, i.e benchmarjs, perf.link, jsben.ch and sbench.me, proposed implementation has generally better performance over current one but from time to time current implementation won.

I was hoping for far more performance gain since we amortize the variable lookup but I guess browser engine does that for both implementations.

[milomg]

This sounds interesting... Happen to have a link to one of the benchmarks?

[snnsnn]

This sounds interesting... Happen to have a link to one of the benchmarks?

https://jsben.ch/imZOH

I don't have https://jsbench.me/ account, could not share links but here is a screenshot:

Some tools errors over export syntax, other have issues with parsing comments so I created a a cleanup versions which could be copy pasted. For this comparison, I renamed all variables in updated version to prevent conflicts and clashes.

let CurrentReaction = undefined;
let CurrentGets = null;
let CurrentGetsIndex = 0;
let EffectQueue = [];

const CacheClean = 0; 
const CacheCheck = 1; 
const CacheDirty = 2; 

function reactive(fnOrValue) {
  return new Reactive(fnOrValue);
}

class Reactive {
  constructor(fnOrValue, effect) {
    this.observers = null; 
    this.sources = null; 
    this.cleanups = [];
    if (typeof fnOrValue === 'function') {
      this.fn = fnOrValue;
      this._value = undefined;
      this.effect = effect || false;
      this.state = CacheDirty;
      if (effect) this.update(); 
    } else {
      this.fn = undefined;
      this._value = fnOrValue;
      this.state = CacheClean;
      this.effect = false;
    }
  }
  get value() {
    return this.get();
  }
  set value(v) {
    this.set(v);
  }
  get() {
    if (CurrentReaction) {
      if (
        !CurrentGets &&
        CurrentReaction.sources &&
        CurrentReaction.sources[CurrentGetsIndex] == this
      ) {
        CurrentGetsIndex++;
      } else {
        if (!CurrentGets) CurrentGets = [this];
        else CurrentGets.push(this);
      }
    }
    if (this.fn) this.updateIfNecessary();
    return this._value;
  }
  set(value) {
    if (this._value !== value && this.observers) {
      for (let i = 0; i < this.observers.length; i++) {
        this.observers[i].stale(CacheDirty);
      }
    }
    this._value = value;
  }
  stale(state) {
    if (this.state < state) {
      if (this.state === CacheClean && this.effect) EffectQueue.push(this);
      this.state = state;
      if (this.observers) {
        for (let i = 0; i < this.observers.length; i++) {
          this.observers[i].stale(CacheCheck);
        }
      }
    }
  } 
  update() {
    const oldValue =
      this
        ._value; 
    const prevReaction = CurrentReaction;
    const prevGets = CurrentGets;
    const prevIndex = CurrentGetsIndex;
    CurrentReaction = this;
    CurrentGets = null; 
    CurrentGetsIndex = 0;
    try {
      if (this.cleanups.length) {
        this.cleanups.forEach((c) => c(this._value));
        this.cleanups = [];
      }
      this._value = this.fn(); 
      if (CurrentGets) {
        this.removeParentObservers(); 
        if (this.sources && CurrentGetsIndex > 0) {
          this.sources.length = CurrentGetsIndex + CurrentGets.length;
          for (let i = 0; i < CurrentGets.length; i++) {
            this.sources[CurrentGetsIndex + i] = CurrentGets[i];
          }
        } else {
          this.sources = CurrentGets;
        }
        for (let i = CurrentGetsIndex; i < this.sources.length; i++) {
          const source = this.sources[i];
          if (!source.observers) {
            source.observers = [this];
          } else {
            source.observers.push(this);
          }
        }
      } else if (this.sources && CurrentGetsIndex < this.sources.length) {
        this.removeParentObservers();
        this.sources.length = CurrentGetsIndex;
      }
    } finally {
      CurrentGets = prevGets;
      CurrentReaction = prevReaction;
      CurrentGetsIndex = prevIndex;
    } 
    if (oldValue !== this._value && this.observers) {
      for (let i = 0; i < this.observers.length; i++) {
        this.observers[i].state = CacheDirty;
      }
    } 
    this.state = CacheClean;
  } 
  updateIfNecessary() {
    if (this.state === CacheCheck) {
      for (const source of this.sources) {
        source.updateIfNecessary(); 
        if (this.state === CacheDirty) {
          break;
        }
      }
    } 
    if (this.state === CacheDirty) {
      this.update();
    } 
    this.state = CacheClean;
  }
  removeParentObservers() {
    if (!this.sources) return;
    for (let i = CurrentGetsIndex; i < this.sources.length; i++) {
      const source = this.sources[i]; 
      const swap = source.observers.findIndex((v) => v === this);
      source.observers[swap] = source.observers[source.observers.length - 1];
      source.observers.pop();
    }
  }
}
function onCleanup(fn) {
  if (CurrentReaction) {
    CurrentReaction.cleanups.push(fn);
  } else {
    console.error('onCleanup must be called from within a @reactive function');
  }
}
function stabilize() {
  for (let i = 0; i < EffectQueue.length; i++) {
    EffectQueue[i].get();
  }
  EffectQueue.length = 0;
}

///////////// UPDATED IMPLEMENTATITON ////////////////
// Variables are renamed, so there is no overlap

let UpdatedReaction = undefined;
let UpdatedGets = null;
let UpdatedGetsIndex = 0;
let UpdatedEffectQueue = [];

function updated(fnOrValue) {
  return new UpdatedReactive(fnOrValue);
}

class UpdatedReactive {
  constructor(fnOrValue, effect) {
    this.observers = null; 
    this.sources = null; 
    this.cleanups = [];
    if (typeof fnOrValue === 'function') {
      this.fn = fnOrValue;
      this._value = undefined;
      this.effect = effect || false;
      this.state = 'DIRTY';
      if (effect) this.update(); 
    } else {
      this.fn = undefined;
      this._value = fnOrValue;
      this.state = 'CLEAN';
      this.effect = false;
    }
  }
  get value() {
    return this.get();
  }
  set value(v) {
    this.set(v);
  }
  get() {
    if (UpdatedReaction) {
      if (
        !UpdatedGets &&
        UpdatedReaction.sources &&
        UpdatedReaction.sources[UpdatedGetsIndex] == this
      ) {
        UpdatedGetsIndex++;
      } else {
        if (!UpdatedGets) UpdatedGets = [this];
        else UpdatedGets.push(this);
      }
    }
    if (this.fn) this.updateIfNecessary();
    return this._value;
  }
  set(value) {
    if (this._value !== value && this.observers) {
      for (let i = 0; i < this.observers.length; i++) {
        this.observers[i].stale('DIRTY');
      }
    }
    this._value = value;
  }
  stale(state) {
   if (this.state === 'CLEAN') {
      if (this.effect) EffectQueue.push(this);
      this.state = state;
      if (this.observers) {
        for (let i = 0; i < this.observers.length; i++) {
          this.observers[i].stale('CHECK');
        }
      }
    }
  } 
  update() {
    const oldValue =
      this
        ._value; 
    const prevReaction = UpdatedReaction;
    const prevGets = UpdatedGets;
    const prevIndex = UpdatedGetsIndex;
    UpdatedReaction = this;
    UpdatedGets = null; 
    UpdatedGetsIndex = 0;
    try {
      if (this.cleanups.length) {
        this.cleanups.forEach((c) => c(this._value));
        this.cleanups = [];
      }
      this._value = this.fn(); 
      if (UpdatedGets) {
        this.removeParentObservers(); 
        if (this.sources && UpdatedGetsIndex > 0) {
          this.sources.length = UpdatedGetsIndex + UpdatedGets.length;
          for (let i = 0; i < UpdatedGets.length; i++) {
            this.sources[UpdatedGetsIndex + i] = UpdatedGets[i];
          }
        } else {
          this.sources = UpdatedGets;
        }
        for (let i = UpdatedGetsIndex; i < this.sources.length; i++) {
          const source = this.sources[i];
          if (!source.observers) {
            source.observers = [this];
          } else {
            source.observers.push(this);
          }
        }
      } else if (this.sources && UpdatedGetsIndex < this.sources.length) {
        this.removeParentObservers();
        this.sources.length = UpdatedGetsIndex;
      }
    } finally {
      UpdatedGets = prevGets;
      UpdatedReaction = prevReaction;
      UpdatedGetsIndex = prevIndex;
    } 
    if (oldValue !== this._value && this.observers) {
      for (let i = 0; i < this.observers.length; i++) {
        this.observers[i].state = 'DIRTY';
      }
    } 
    this.state = 'CLEAN';
  } 
  updateIfNecessary() {
    if (this.state === 'CHECK') {
      for (const source of this.sources) {
        source.updateIfNecessary(); 
        if (this.state === 'DIRTY') {
          break;
        }
      }
    } 
    if (this.state === 'DIRTY') {
      this.update();
    } 
    this.state = 'CLEAN';
  }
  removeParentObservers() {
    if (!this.sources) return;
    for (let i = UpdatedGetsIndex; i < this.sources.length; i++) {
      const source = this.sources[i]; 
      const swap = source.observers.findIndex((v) => v === this);
      source.observers[swap] = source.observers[source.observers.length - 1];
      source.observers.pop();
    }
  }
}
function updatedOnCleanup(fn) {
  if (UpdatedReaction) {
    UpdatedReaction.cleanups.push(fn);
  } else {
    console.error('onCleanup must be called from within a @reactive function');
  }
}
function updatedstabilize() {
  for (let i = 0; i < UpdatedEffectQueue.length; i++) {
    UpdatedEffectQueue[i].get();
  }
  UpdatedEffectQueue.length = 0;
}

// Current implementation
const currentCounter = reactive(0);
currentCounter.set(5);
reactive(() => (currentCounter.value & 1) == 0);

// Updated implementation
const updatedCounter = updated(0);
updatedCounter.set(5);
updated(() => (updatedCounter.value & 1) == 0);

[milomg]

I like the idea of using strings unions to make things clearer and potentially faster, and that is a common technique in JavaScript to make APIs explicit.

In TypeScript however, we have the luxury of using types which get compiled away. If you're interested, one alternative to prevent using 0 in the code (a compiler should still optimize this away) would be to add type brands to typescript.

In a lower level language I would expect that the cost of strings is higher than reading a variable, and that is what I see here: those benchmarks show the current implementation being slightly faster and slightly smaller than new implementation for me.

[snnsnn]

one alternative to prevent using 0 in the code (a compiler should still optimize this away) would be to add type brands to typescript.

I don't know that method. Do you mean named enums like:

enum Cache {
  Clean = 'Clean',
  Dirty = 'Dirty',
  Check = 'Check',
}

Enums in TS produce very verbose output and performance degrades considerably and it is true for C like enums. I believe you intentionally avoid them and go with unions.

var Cache;
(function (Cache) {
    Cache["Clean"] = "Clean";
    Cache["Dirty"] = "Dirty";
    Cache["Check"] = "Check";
})(Cache || (Cache = {}));

Actually my initial intention was providing an enum like the one above, but realized its performance cost after running some test and switched to proposed implementation.

Rescript avoids this pitfall and compiles to very clean code:

type cache =
  | Clean
  | Dirty
  | Check

let x: cache = Clean;

Which outputs:

var x = 0;

[snnsnn]

I have a question about stale checking:

  private stale(state: CacheNonClean): void {

    // Does this means `this.state === 'CLEAN'`
    // since 0 | 1 | 2 < 1 | 2 and CacheWithoutClean is never 0
    if (this.state < state) { }
  }

This makes inner conditional if (this.state === CacheClean && this.effect) EffectQueue.push(this); redundant hence practically equivalent to:

  private stale(state: CacheWithoutClean): void {
    if (this.state === 'CLEAN') {
      // If we were previously clean, then we know that we may need to update to get the new value
      if (this.effect) EffectQueue.push(this);

      this.state = state;

      if (this.observers) {
        for (let i = 0; i < this.observers.length; i++) {
          this.observers[i].stale('CHECK');
        }
      }
    }
  }

[milomg]

That isn't quite correct. The first marking of stale takes nodes from CHECK to DIRTY (this.state == 1 && state == 2) (I would expect that running the test suite should catch this, if it doesn't I'd love to know)

[snnsnn]

I would expect that running the test suite should catch this, if it doesn't I'd love to know

Unfortunately it did not, all test are passing.

Test Suites: 7 passed, 7 total
Tests:       5 skipped, 35 passed, 40 total

The first marking of stale takes nodes from CHECK to DIRTY (this.state == 1 && state == 2)

Does not this conditional (this.state == 1 && state == 2) make the inner conditional if (this.state === CacheClean && this.effect) always false? Typescript returns type error for it.

  private stale(state: CacheWithoutClean): void {
    if (this.state === 'CHECK' && state === 'DIRTY') {

      // TYPE ERROR: This condition will always return 'false' since the types '"CHECK"' and '"CLEAN"' have no overlap.ts(2367)
      if (this.state ==='CLEAN' && this.effect) EffectQueue.push(this);

      this.state = state;

      if (this.observers) {
        for (let i = 0; i < this.observers.length; i++) {
          this.observers[i].stale('CHECK');
        }
      }
    }
  }