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

Should browsers do bookkeeping on DOM parts after initial parse? #1030

Open
iteriani opened this issue Sep 15, 2023 · 8 comments
Open

Should browsers do bookkeeping on DOM parts after initial parse? #1030

iteriani opened this issue Sep 15, 2023 · 8 comments

Comments

@iteriani
Copy link

See preliminary benchmark in https://playcode.io/1595899 using chrome-dev with experimental platform features on.

My numbers appear to be

cloning
8.100000023841858
getParts
1.5000000447034836
accessNodes
1.0999999791383743
operations
1.800000011920929
replaceChildren
30.5

As you can see, there is huge overhead in replaceChildren, presumably to do bookkeeping on keeping the parts up to date. Given how we expect users to use DOM parts in frameworks, we should think about whether bookkeeping is necesary in v1.

@rniwa
Copy link
Collaborator

rniwa commented Sep 15, 2023

What kind of bookkeepings are there?

@iteriani
Copy link
Author

iteriani commented Sep 15, 2023

Consider the following test case.

const el = template`<div>{{#}}{{/#}}</div>`;
const child = template`<span>{{#}}Something here{{/#}}</span>`;
const part = el.content.getPartRoot().getParts()[0];
expect(part.getParts().length).toBe(0);
part.replaceChildren(child);
expect(part.getParts().length).toBe(1);

I personally would be ok with the last assertion to be 0.

@rniwa
Copy link
Collaborator

rniwa commented Sep 15, 2023

Why is supporting getParts() to be updated so expensive? It seems like an implementation problem to me.

@iteriani
Copy link
Author

iteriani commented Sep 15, 2023 via email

@mfreed7
Copy link

mfreed7 commented Sep 15, 2023

Why is supporting getParts() to be updated so expensive? It seems like an implementation problem to me.

I tend to agree - I’d refrain from making API decisions too early based on the early prototype. I haven’t had much time to optimize and it’s likely there’s something easy that could be fixed in replaceChildren.

That said, it’d be good to have the debate about whether tracking is necessary, because it certainly has non-zero overhead. Removing it will make the basic operations faster. But I don’t know if removing it will make the usage of the API faster, since it might require more work to be done on the framework side.

@justinfagnani
Copy link
Contributor

If bookkeeping does end up being irreducibly expensive, I was definitely partial to the template.cloneWithParts() API before, which essentially required any bookkeeping to be done on the JS side. If you don't have uncontrolled DOM mutations, where you then have to ask for the current parts, this seems fine.

@iteriani
Copy link
Author

iteriani commented Sep 16, 2023

Mason fixed up my benchmark a little bit, so now we're a little bit better with DOM parts being about 1.3x slower (can definitely be investigated further).

See https://playcode.io/1595899

Screenshot 2023-09-16 at 7 03 08 AM

@mfreed7
Copy link

mfreed7 commented Sep 16, 2023

can definitely be investigated further

…is actively being investigated further…

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

No branches or pull requests

5 participants
@rniwa @justinfagnani @iteriani @mfreed7 and others