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.

Sign up for GitHub

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

Jump to bottom

[BUG] built is duplicating "current" in the array for every child element #186

Closed
LionsAd opened this issue Nov 24, 2020 · 4 comments · May be fixed by #187
Closed

[BUG] built is duplicating "current" in the array for every child element #186

LionsAd opened this issue Nov 24, 2020 · 4 comments · May be fixed by #187
Labels
discussion Discussions and proposals needs more information

Comments

@LionsAd
Copy link

LionsAd commented Nov 24, 2020

To reproduce in JS console with built.mjs pasted in:

let Tab = function() { return 'Hi'; }
let index = 1;
let href='/';
let onMouseDown='';
let isAction = '';

build`<${Tab} index=${index} key=${href} href=${href} onMouseDown=${onMouseDown} isActive=${isActive}>
        ${title}
        <span>Hi</span>
      <//>`;

Expected output:

APPEND_CHILD, TAG_SET, PROP_SET, PROP_SET, CHILD_RECURSIVE

Real output:

    [0] => Array
        (
            [0] => 0
            [1] => 2
            [2] => 0
            [3] => Array
                (
                    [0] => Array
                        (
                            [0] => 0
                        )

                    [1] => 3
                    [2] => 1
                    [3] => 
                    [4] => 5
                    [5] => 2
                    [6] => 0
                    [7] => index
                    [8] => 5
                    [9] => 3
                    [10] => 0
                    [11] => key
                    [12] => 5
                    [13] => 4
                    [14] => 0
                    [15] => href
                    [16] => 5
                    [17] => 5
                    [18] => 0
                    [19] => onMouseDown
                    [20] => 5
                    [21] => 6
                    [22] => 0
                    [23] => isActive
                    [24] => 0
                    [25] => 7
                    [26] => 
                    [27] => 0
                    [28] => 0
                    [29] =>         
                    [30] => 2
                    [31] => 0
                    [32] => Array
                        (
                            [0] => Array
                                (
                                    [0] => Array
                                        (
                                            [0] => 0
                                        )

                                    [1] => 3
                                    [2] => 1
                                    [3] => 
                                    [4] => 5
                                    [5] => 2
                                    [6] => 0
                                    [7] => index
                                    [8] => 5
                                    [9] => 3
                                    [10] => 0
                                    [11] => key
                                    [12] => 5
                                    [13] => 4
                                    [14] => 0
                                    [15] => href
                                    [16] => 5
                                    [17] => 5
                                    [18] => 0
                                    [19] => onMouseDown
                                    [20] => 5
                                    [21] => 6
                                    [22] => 0
                                    [23] => isActive
                                    [24] => 0
                                    [25] => 7
                                    [26] => 
                                    [27] => 0
                                    [28] => 0
                                    [29] =>         
                                )

                            [1] => 3
                            [2] => 0
                            [3] => span
                            [4] => 0
                            [5] => 0
                            [6] => Hi
                        )

                )

        )

This is from my PHP port of preact (just an experiment), but it also happens with preact master in Javascript.

I think when recursing into the children, the data should be stored in a stack variable and not in current[0].

While for traversing that's not a problem, it increases the size of current dramatically and this - unless cleaned up - is what is cached, which is also not ideal as the sparse data should be as small as possible obviously.

To fix:

The fix is trivial also:

htm/src/build.mjs

Line 262 in ed1c620

mode = current;

be replaced with:

	        mode = current;
                current_0 = current[0];
                mode[0] = [];
				if (MINI) {
					(current = current_0).push(h.apply(null, mode.slice(1)));
				}
				else {
					(current = current_0).push(CHILD_RECURSE, 0, mode);
				}

should be all that is needed. Also could fix the double usage of the name mode at the same time. Likely tmp or current_child or such might be better.
@LionsAd LionsAd mentioned this issue Nov 24, 2020
Closed
LionsAd added a commit to LionsAd/htm that referenced this issue Nov 24, 2020
@LionsAd
Issue developit#186: Fix duplicate data in cached build.
260c05d 
Fixes: developit#186
@LionsAd LionsAd mentioned this issue Nov 24, 2020
Open
@developit
Copy link
Owner

@LionsAd finally getting back to this - any chance you're seeing a circular reference in JS but the PHP port happens to clone the Array making it grow? 'm still digging into around, but on master it appears to be a shared array reference that gets reset.

@LionsAd
Copy link
Author

LionsAd commented Dec 15, 2020

Oh - that might very well be. I saw it growing in the JS array as well, but did not check memory usage when memo‘ed.

I will double check that memory usage actually grows.

Thanks for checking that.

@developit
Copy link
Owner

No problem! Sorry I didn't have a definitive answer - I still need to refamiliarize myself with the current codebase 😅

@developit developit added discussion Discussions and proposals needs more information labels Feb 5, 2021
@jviide
Copy link
Collaborator

jviide commented Feb 19, 2021 •
edited

This is intentional. We use current to store the parsing state for the currently processed element, but current also doubles as a parsing stack of sorts: current[0] always points to the parent element's parsing state. When we do current = current[0] we essentially "pop" the stack and go back to processing the parent element's state.

Therefore each current[0] contains just a pointer to an already existing array - the parent's parsing state - and shouldn't take extra memory. In fact, doing mode = current; current_0 = current[0]; mode[0] = []; may take more memory, as it allocates a new array.

Tangentially related: After the whole tree has been parsed the "stack" functionality isn't used anymore, so in evaluate we use current[0] for storing bitflags for identifying static subtrees.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
discussion Discussions and proposals needs more information
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Issue #186: Fix duplicate data in cached build. LionsAd/htm
3 participants
@developit @LionsAd @jviide