fix invalidate while update

[tanhauhau]

Fix #4098

the dirty bitmask passed into the p function will be a snapshot of the dirty accumulated so far.
reset the dirty bitmask before running p function, any $$invalidate call within the p function will schedule another update function.

[ghost]

I think it's much more appropriate to make changes to the way we handle action callbacks, since that is the only place that we've seen invalidates get lost.

This PR is more of a "salt-the-earth" approach that could have serious performance impacts.

I'll mock up an example of the generated code I had in mind so we all can discuss further on Discord and figure out how to convince the compiler to do what we need.

[ghost]

This will cause update to refresh everything, whether changed or not.
Also, we need to be sure that the action callback is only triggered once.

[ghost]

I thought of another oops... what if the 32nd element is invalidated?
We'd lose the bit since the special [-1] is not handled inside p.

[tanhauhau]

only have 31 bit per item in the array, if i understand correctly,
https://github.com/sveltejs/svelte/blob/master/src/compiler/compile/render_dom/Renderer.ts#L216-L217

[ghost]

@tanhauhau,

I think I'm losing it because I thought I saw the fragment update called twice but it is only once.... so nevermind, you are absolutely correct here.

Since this change exists in the other PR, we should close this out.

[tanhauhau]

understand your concern.
not sure whether action callbacks is the only place we need to take care of.

[Conduitry]

Can we safely revert the changes in #4095 as part of this? I'd like to be able to bring back destructuring whenever possible.

As for whether actions are the only place we need to worry about: I'd like to hear what Rich thinks about that - but, since he thought there was no way updates could invalidate additional things, it makes sense to assume that this is the only one unless we bump into another one in the future.

[tanhauhau]

yes. @Conduitry reverted in a separate PR #4102 to keep this one clean, to show what's changed.

[tanhauhau]

actually i feel this change is quite safe and clean.

if there's no $$invalidate calls within the p function, then should have no behavior change, since we are going to reset the $$.dirty to [-1] afterwards anyway.

if there is, it should be thought of the update in the next update cycle, which makes things cleaner to think about, ie $$.dirty shouldn't be changed within p function.

[ghost]

Sorry to post such a large counter-example, but...

Given this Svelte code:

<script>
    import {writable} from 'svelte/store';
    let s = writable("simple");
    let v = "";

    function action(node, binding) {
        return {
            update: (value) => { s.set(value); }
        }
    }
</script>
{$s}
<input bind:value={v} use:action={v}>

Will result in this being generated for p (ignore the dev stuff):

p: function update(ctx, dirty) {
    if (dirty[0] & /*$s*/ 2) set_data_dev(t0, /*$s*/ ctx[1]);

    if (dirty[0] & /*v*/ 1 && input.value !== /*v*/ ctx[0]) {
        set_input_value(input, /*v*/ ctx[0]);
    }

    if (is_function(action_action.update) && dirty[0] & /*v*/ 1) action_action.update.call(null, /*v*/ ctx[0]);
},

Your change will have the side effect of calling the action twice in the same update cycle.

Now, let's say I add 31 more variables, the code becomes:

    		p: function update(ctx, dirty) {
    			if (dirty[1] & /*$s*/ 4) set_data_dev(t0, /*$s*/ ctx[33]); // STORE
    			if (dirty[0] & /*x0*/ 1) set_data_dev(t2, /*x0*/ ctx[0]);
    			if (dirty[0] & /*x1*/ 2) set_data_dev(t3, /*x1*/ ctx[1]);
    			if (dirty[0] & /*x2*/ 4) set_data_dev(t4, /*x2*/ ctx[2]);
    			if (dirty[0] & /*x3*/ 8) set_data_dev(t5, /*x3*/ ctx[3]);
    			if (dirty[0] & /*x4*/ 16) set_data_dev(t6, /*x4*/ ctx[4]);
    			if (dirty[0] & /*x5*/ 32) set_data_dev(t7, /*x5*/ ctx[5]);
    			if (dirty[0] & /*x6*/ 64) set_data_dev(t8, /*x6*/ ctx[6]);
    			if (dirty[0] & /*x7*/ 128) set_data_dev(t9, /*x7*/ ctx[7]);
    			if (dirty[0] & /*x8*/ 256) set_data_dev(t10, /*x8*/ ctx[8]);
    			if (dirty[0] & /*x9*/ 512) set_data_dev(t11, /*x9*/ ctx[9]);
    			if (dirty[0] & /*x10*/ 1024) set_data_dev(t12, /*x10*/ ctx[10]);
    			if (dirty[0] & /*x11*/ 2048) set_data_dev(t13, /*x11*/ ctx[11]);
    			if (dirty[0] & /*x12*/ 4096) set_data_dev(t14, /*x12*/ ctx[12]);
    			if (dirty[0] & /*x13*/ 8192) set_data_dev(t15, /*x13*/ ctx[13]);
    			if (dirty[0] & /*x14*/ 16384) set_data_dev(t16, /*x14*/ ctx[14]);
    			if (dirty[0] & /*x15*/ 32768) set_data_dev(t17, /*x15*/ ctx[15]);
    			if (dirty[0] & /*x16*/ 65536) set_data_dev(t18, /*x16*/ ctx[16]);
    			if (dirty[0] & /*x17*/ 131072) set_data_dev(t19, /*x17*/ ctx[17]);
    			if (dirty[0] & /*x18*/ 262144) set_data_dev(t20, /*x18*/ ctx[18]);
    			if (dirty[0] & /*x19*/ 524288) set_data_dev(t21, /*x19*/ ctx[19]);
    			if (dirty[0] & /*x20*/ 1048576) set_data_dev(t22, /*x20*/ ctx[20]);
    			if (dirty[0] & /*x21*/ 2097152) set_data_dev(t23, /*x21*/ ctx[21]);
    			if (dirty[0] & /*x22*/ 4194304) set_data_dev(t24, /*x22*/ ctx[22]);
    			if (dirty[0] & /*x23*/ 8388608) set_data_dev(t25, /*x23*/ ctx[23]);
    			if (dirty[0] & /*x24*/ 16777216) set_data_dev(t26, /*x24*/ ctx[24]);
    			if (dirty[0] & /*x25*/ 33554432) set_data_dev(t27, /*x25*/ ctx[25]);
    			if (dirty[0] & /*x26*/ 67108864) set_data_dev(t28, /*x26*/ ctx[26]);
    			if (dirty[0] & /*x27*/ 134217728) set_data_dev(t29, /*x27*/ ctx[27]);
    			if (dirty[0] & /*x28*/ 268435456) set_data_dev(t30, /*x28*/ ctx[28]);
    			if (dirty[0] & /*x29*/ 536870912) set_data_dev(t31, /*x29*/ ctx[29]);
    			if (dirty[0] & /*x30*/ 1073741824) set_data_dev(t32, /*x30*/ ctx[30]);
    			if (dirty[1] & /*x31*/ 1) set_data_dev(t33, /*x31*/ ctx[31]);

    			if (dirty[1] & /*v*/ 2 && input.value !== /*v*/ ctx[32]) { // THE ORIGINAL VARIABLE
    				set_input_value(input, /*v*/ ctx[32]);
    			}

    			if (is_function(action_action.update) && dirty[1] & /*v*/ 2) action_action.update.call(null, /*v*/ ctx[32]);
    		},

Your change will now cause every one of them to be refreshed in the DOM, even though only v and the store have changed. You'll also notice that your original goal of getting {$s} to update fails because the array only contains a single a mask at dirty[0], which breaks the dirty[1] checks.

[ghost]

Here's my idea, although not perfectly efficient because it re-updates anything that had a bitmask set initially.

It can be improved by taking a snapshot as previous suggested and masking off the bits already taken care of just before calling the actions. We wouldn't have to track a weight w in that case and just return the result of dirty.reduce(sum).

Caveat:
I'll have to think about how to accomplish the same thing with a destructuring in place.

Given this Svelte code:

<script>
    import {writable} from 'svelte/store';
    let s = writable("simple");
    let v = "";

    function action(node, binding) {
        return {
            update: (value) => { s.set(value); }
        }
    }
</script>
{$s}
<input bind:value={v} use:action={v}>

Generate this:

function update($$) {
    if ($$.fragment !== null) {
        let r; // ADD
        $$.update();
        run_all($$.before_update);
        $$.fragment && (r = $$.fragment.p($$.ctx, $$.dirty)); // CHANGE
        r && $$.fragment.p($$.ctx, $$.dirty, r); // ADD
        $$.dirty = [-1];
        $$.after_update.forEach(add_render_callback);
    }
}

...and this...

// declared in instance()
const sum = (t,v) => t+v;

p: function update(ctx, dirty, reenter) {
    if (dirty[0] & /*$s*/ 2) set_data_dev(t0, /*$s*/ ctx[1]);

    if (dirty[0] & /*v*/ 1 && input.value !== /*v*/ ctx[0]) {
        set_input_value(input, /*v*/ ctx[0]);
    }

    // start of actions
    // (wrapped to protect against re-execution)

    if(!reenter) {
        const w = dirty.reduce(sum);
        if (is_function(action_action.update) && dirty[0] & /*v*/ 1) action_action.update.call(null, /*v*/ ctx[0]);
        // end of action code

        return w < dirty.reduce(sum);
    }
},

[tanhauhau]

oh i roughly understand your concern now. yes, -1 is 0b1111111111...1 in binary, but it is actually a special case as a default value for the $dirty bit. setting it to -1 will not reschedule an update. but calling $$invalidate while it is -1 will,
see

svelte/src/runtime/internal/Component.ts

Lines 90 to 93 in 54e8037

	function make_dirty(component, i) {
	if (component.$$.dirty[0] === -1) {
	dirty_components.push(component);
	schedule_update();

[ghost]

@tanhauhau,

Yeah, I see in #4102, you save off dirty, set the component dirty to the special case, and then call update with the saved mask.

Let me try that branch on my test cases here and see what happens (which I'm sure you already did).

EDIT:
I see you added the store test case there as well as some others.
Things seem to be working fine, so I don't think this PR is needed at all!

Great work!!!