replaceUndefinedAttributes causes superlinear time complexity after small additions to the page

[evnchn]

TL-DR (by ChatGPT):

I noticed that appending 20 elements to the page was taking longer than expected, with increasing delays during each operation. I timed the update() function and discovered that replaceUndefinedAttributes() was consuming most of the time. This function recursively processes element slots, which is problematic, especially in Quasar with its numerous slots.

To improve performance, I developed three potential solutions:

Cache elements that have already had replaceUndefinedAttributes called and skip it for those.
Disable replaceUndefinedAttributes entirely and make the calling code handle undefined attributes safely.
Disable it and check for undefined attributes in the calling code, fixing them as needed.
I've modified nicegui.js to implement these ideas, which seems to improve loading speed on the test pages. However, I want to ensure that there are no critical reasons for populating all attributes with non-undefined values before finalizing any changes.

Reference to:
#4434 about "How to make the page load faster, or add a waiting prompt"
https://github.com/evnchn/nicegui-speed-investigation (Test page used here: /native/after_conn/js)

So I was like:
Hmm... So I am appending 20 elements to the page, well each operation should be O(1) with regards to page length n, right? But why does it take longer and longer, with the spinner stuttering more and more on both methods???

I went ahead and timed the update() function which was invoked on every append. (In retrospect, I should have put the time marks better, but anyways...)

I was surprised to find out that replaceUndefinedAttributes() was taking up a majority of the time!.

nicegui/nicegui/static/nicegui.js

Line 38 in a366de0

Object.values(element.slots).forEach((slot) => slot.ids.forEach((id) => replaceUndefinedAttributes(elements, id)));

Initiating the attributes of one element should be pretty harmless, but then it also does that recursively on element.slots

My immediate thought was, wait, doesn't Quasar have a lot of slots? That could take ages.

Indeed, after a bit of mucking around, I managed to come up with 3 solutions, 2 of which you can find traces of in my modified nicegui.js

1. cache elements for which replaceUndefinedAttributes has been called with already, and skip calling replaceUndefinedAttributes if that is the case.
2. Flat-out disable replaceUndefinedAttributes, and code which supposed to access the elements of the attributes are responsible for handling it safely (e.g. (element.events || []).forEach((event) => {, because forEach on undefined is an error).
3. Flat-out disable replaceUndefinedAttributes, and code which supposed to access the elements of the attributes can check that it is undefined, and then fix it themselves.

Here's the modified nicegui.js, which I can already see can error out elsewhere, but at least works on the nicegui-speed-investigation test pages.
https://gist.github.com/evnchn/1aa7304f635d5a599137a66f66ee04c0

Here's the diff between my modded version and the version found in 2.12.1
diff-nicegui.js.pdf

Here's the loading speed after all that modification

Before PR, I want to check and see if there are any reasons for populating all attributes with non-undefined values, so that I don't make a fool of myself breaking something else. Though, I would say, I can't see how it would.

@falkoschindler Sorry for the mention, but I believe this may be important.

[evnchn]

And, we may be able to requestAnimationFrame to get rid of all of the lag too!

[falkoschindler]

That's an interesting observation, @evnchn!

If I understand correctly: When adding element number 21 to a container, an update message with the container as well as this new element is sent to the client. There replaceUndefinedAttributes is called for the container (recursively processing all its 21 children) as well as for the new element (useless, because it has already been processed). This can indeed be improved.

Regarding your options: Caching element would be a viable solution, but of course adds a bit complexity to it. Removing replaceUndefinedAttributes and handling undefined attributes on demand seems also reasonable. But when introducing these undefined attributes in #2702 to reduce payload, we wanted to recover the original attributes in a central place so that we don't miss a spot. So I lean towards option 1 because it keeps the responsibility for having all attributes defined in one place. If done incorrectly, it can lead to very nasty bugs (see #2957 (comment) for example).

Would you like to create a pull request? Then we can discuss further implementation details more easily.

Oh and regarding requestAnimationFrame: I'm not sure what you mean. Maybe this is a topic for a separate discussion?