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

Possible size reduction #359

Open
X-Ryl669 opened this issue Aug 30, 2024 · 8 comments
Open

Possible size reduction #359

X-Ryl669 opened this issue Aug 30, 2024 · 8 comments

Comments

@X-Ryl669
Copy link

X-Ryl669 commented Aug 30, 2024

In this code
you can avoid the dom.remove() by simply doing dom.replaceWith('')
So it can be replaced as
let update = (dom, newDom) => newDom === dom || dom.replaceWith(newDom||'')

Also, there is many time the document variable in the script. Maybe it's worth creating an alias, since it won't be renamed by the minifier code, like this :

let dom = ns ? document.createElementNS(ns, name) : document.createElement(name)
// instead:
let d=document,dom = ns ? d.createElementNS(ns, name) : d.createElement(name)
// Or better:
let n = ns&&'NS', doc = document['createElement'+n](ns||name,name)

And finally, k.startsWith("on") appears twice in the same method, and is longer than an alias let o=k.startsWith('on') so it can be further optimized away.

Anyway, thanks for the library, it's very useful.

@Tao-VanJS
Copy link
Member

Thanks @X-Ryl669 for the suggestion!

I think lots of size optimization ideas can make the .min.js bundle smaller, but will indeed increase the gzipped bundle size. This is because gzip compression will compress the repetitive patterns in the input text and some aliasing optimization will eliminate the repetitive patterns in the code.

For instance, I tried to apply the ideas of aliasing the result of k.startsWith("on"), it will indeed increase the gzipped bundle size by 1 byte to 1049 bytes. Thus we need to try out every optimization idea to assess its ultimate impact on the gzipped bundle size.

@sirenkovladd
Copy link
Contributor

sirenkovladd commented Aug 30, 2024

@X-Ryl669
If you have enough time you can play around with these suggestions, I was able to find a couple of bytes of optimization by changing the order of variables or variable names
but I don't think it's worth it.

is a useful tool: https://github.com/andrewiggins/gz-heatmap

@Tao-VanJS
Copy link
Member

Also,

dom.replaceWith('')

is not equivalent to

dom.remove()

as the former will leave an empty text node underneath the DOM tree.

@X-Ryl669
Copy link
Author

Is it really a problem ?

@Tao-VanJS
Copy link
Member

Is it really a problem ?

It can be a problem. Think about the use case like a TODO list, where lots of items are added to the list and then deleted. Eventually it will leave lots of dummy nodes in the DOM tree. Thus this is basically a kind of memory leak.

@sirenkovladd
Copy link
Contributor

Actually, I tried to use replaceWith('') for chromium based browser and this element does not stay in the DOM tree
children elements are reduced

Do you have any example of a memory leak being possible?

@Tao-VanJS
Copy link
Member

Do you have any example of a memory leak being possible?

Please see this example: https://jsfiddle.net/qsvz2abc/2/ (a modified TODO list).

Basically, if we change .remove() to .replaceWith(""). The total count of the child nodes of the TODO list won't be decreased when a TODO item is removed.

@sirenkovladd
Copy link
Contributor

Thanks a lot, when I looked before I noticed that replaceWith reduces the number of children
but didn't notice that it doesn't reduce the number of childNodes

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

3 participants