Skip to content

[WIP] implement bitmask-based change tracking #1943

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

Closed
wants to merge 1 commit into from
Closed

Conversation

Rich-Harris
Copy link
Member

Just some test output for now — manually tweaked all the expected.js files to help me think about the problem a bit more. Couple of takeaways:

  • I was wrong about being able to represent up to 53 values in a bitmask. JS bitwise operators work in 32 bits, which means we can represent up to 31 values. I don't know if that's a dealbreaker
  • If a component only has one reactive value (where 'reactive' means 'referenced in the template or in a reactive declaration'), there's no need for any change tracking — if changed === 0 then we can skip updates altogether, which means that if an update is happening then that single reactive value definitely has changed
  • Since changed is therefore an optional argument to update functions, the order of arguments should be reversed — (ctx, changed) => {...} rather than (changed, ctx) => {...} — so that we can omit it where possible.
  • n & -1 is only 0 when n === 0 (in fact, for integers up to a certain size, n & -1 === n). This gives us an easy way to say 'everything changed,' e.g. when initialising reactive declarations

We might be able to use that insight to reduce duplication between create and update code. If you have a template like this...

<h1>Hello {name.split('').reverse().reverse().join('').toUpperCase()}!</h1>

...that expression is output twice:

function create_fragment($$, ctx) {
  var h1, text0, text1_value = ctx.name.split('').reverse().reverse().join('').toUpperCase(), text1, text2, current;

  return {
    c() {
      h1 = createElement("h1");
      text0 = createText("Hello ");
      text1 = createText(text1_value);
      text2 = createText("!");
    },

    // ...

    p(changed, ctx) {
      if ((changed.name) && text1_value !== (text1_value = ctx.name.split('').reverse().reverse().join('').toUpperCase())) {
        setData(text1, text1_value);
      }
    },

    // ...
  };
}

Usually we're not duplicating code as contrived as that, but it's a problem nonetheless. A neater version would look like this:

function create_fragment($$, ctx) {
-  var h1, text0, text1_value = ctx.name.split('').reverse().reverse().join('').toUpperCase(), text1, text2, current;
+  var h1, text0, text1, text1_value, text2, current;

  return {
    c() {
      h1 = createElement("h1");
      text0 = createText("Hello ");
-     text1 = createText(text1_value);
+     text1 = createText();
      text2 = createText("!");
+     this.p(ctx, -1);
    },

    // ...

-    p(changed, ctx) {
+    p(ctx, changed) {
-      if ((changed.name) && text1_value !== (text1_value = ctx.name.split('').reverse().reverse().join('').toUpperCase())) {
+      if (changed & /*name*/1 && text1_value !== (text1_value = ctx.name.split('').reverse().reverse().join('').toUpperCase())) {
        setData(text1, text1_value);
      }
    },

    // ...
  };
}

I had a run that problem a while back but it proved too difficult, particularly around instantiating nested components. It might be easier now.

Also, at present, the $$dirty object passed into the reactive declaration update block (which is the same as changed elsewhere; we should make that consistent) is conveniently mutable, which means that sequential updates work:

<script>
  export let a = 1;
  let b, c, d;
	
  $: b = a + 1;
  $: c = b + 1;
  $: d = c + 1;
</script>
// becomes...
$$self.$$.update = ($$dirty = { a: 1, b: 1, c: 1 }) => {
  if ($$dirty.a) {
    b = a + 1; $$invalidate('b', b);
  }
  if ($$dirty.b) {
    c = b + 1; $$invalidate('c', c);
  }
  if ($$dirty.c) {
    d = c + 1; $$invalidate('d', d);
  }
};

Here, $$dirty.b is true because of the $$invalidate('b', b) right before it. If $$dirty isn't an object that would no longer work. So I guess we'd need to do this instead:

-$$self.$$.update = ($$dirty = { a: 1, b: 1, c: 1 }) => {
+$$self.$$.update = ($$dirty = -1) => {
-  if ($$dirty.a) {
-    b = a + 1; $$invalidate('b', b);
+  if ($$dirty & /*a*/1) {
+    b = a + 1; $$invalidate('b', 1, b); $$dirty |= 2;
  }
-  if ($$dirty.b) {
-    c = b + 1; $$invalidate('c', c);
+  if ($$dirty & /*b*/2) {
+    c = b + 1; $$invalidate('c', 2, c); $$dirty |= 4;
  }
-  if ($$dirty.c) {
-    d = c + 1; $$invalidate('d', d);
+  if ($$dirty & /*c*/4) {
+    d = c + 1; $$invalidate('d', 3, d); $$dirty |= 8;
  }
+  return $$dirty;
};

@Rich-Harris
Copy link
Member Author

General consensus was that the 31 limit is untenable. So for this to work we'd need to be able to have multiple bitmasks for components that were big enough — something like

p(ctx, changed1, changed2) {
  if (changed1 & /*foo*/8 || changed2 & /*bar*/4) {
    // ...
  }
}

or

p(ctx, changed) {
  if (changed.a & /*foo*/8 || changed.b & /*bar*/4) {
    // ...
  }
}

With that, this would no longer be a breaking change, which means we don't need to get it done for v3. So I'm going to park this for now.

@zdmitr
Copy link

zdmitr commented Jun 26, 2019

What about Big int for such case?

@Rich-Harris
Copy link
Member Author

Unfortunately BigInt would break compatibility with older browsers. It's possible to transpile them, but the overhead it introduces makes it untenable.

Rich-Harris added a commit that referenced this pull request Nov 23, 2019
* start updating tests

* start implementing bitmask-based change tracking (#1943)

* oops

* fix some await block stuff

* slots

* reactive declarations

* component bindings etc

* start fixing slots

* fix store value invalidations

* slot stuff

* fixes

* fix

* fixes

* fix some slot stuff

* fix some invalidations

* fix if blocks

* fix a test

* destructuring in lets

* fix shadowing

* fix if block case

* all runtime tests passinfg

* almost all tests passing

* update tests

* never hoist writable vars in dev mode, fix debug statements

* beef up shadowing test

* always use renderer.reference

* fix sourcemaps

* ugh so close

* all tests passing. phase one complete, i guess

* add test for component with more than 31 dynamic values

* stable sort

* stable sort that preserves order

* linting

* failing test for bitmask overflow

* ok i think this is it

* lint

* rename changed to dirty, for more internal consistency

* update tests

* use bitwise comparison

* add comments... sort of

* update tests

* moar comments

* I don't know what happened to these tests
@Rich-Harris Rich-Harris deleted the gh-1922 branch November 24, 2019 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants