Many eventhandlers slow page load significantly

[Grelamr]

Summary: Pages with a large number of eventhandlers load slowly, because of frequent, expensive SHA256 hash computation

Given the following “complex“ page, which takes roughly 2-3 seconds of servertime per click. Most of this time is spent while creating the Buttons

@Route("")
public class MainView extends VerticalLayout {
    VerticalLayout container = new VerticalLayout();
    public MainView() {
        Button btn = new Button("refresh");
        btn.addClickListener(e->{
            container.removeAll();
            for (int i = 0; i <1000; i++) {
                container.add(new H1("Headline " + i));
                container.add(new Button("BTN " + i, e2 -> Notification.show("clicked")));
            }
            UI.getCurrent().getPage().executeJs("setTimeout(function(){$0.click()},2000)", btn);
        });
        add(btn,container);
    }
}

Flow version: 14.1.19
Expected behaviour: fast pageload
Actual behaviour: slow pageload

Profiling shows the following results:

• Creating a Button takes 12x the time of creating a H1
• most this time is spent in the ConstantPoolKey.calculateHash method
  • half of which is spent on creating a string from the object
  • the other half is used to calculate the sha256 of this sting
• 8 % of the total time is spent on MessageDigest.getInstance

Ideas:

• MessageDigest-Instances should be reused to avoid unnecessary and expensive instantiation on every call
  • MessageDigest is not threadsafe. It could be cached per UI-Thread: "After digest has been called, the MessageDigest object is reset to its initialized state."
• Is sha256 (or any other cryptographic hash) really required?
  The result is truncated to 64 bits anyways
• Is the ConstantPool (introduced in Introduce a constant pool for shared immutable state node values #1083) required at all?
  Compression should take care of repeated elements.

[Legioth]

• Compression does not take care of repeated elements between multiple HTTP connection, i.e. separate round trips. Persistent connections (HTTP2 or classical HTTP keepalive) is to some degree helping here as well. Would be good to investigate to what degree.
• sha256 is not strictly necessary, but something with better collision resistance than Java's own String.hashCode would still be great.

Some other ideas that could be considered

• Use the constant pool only for longer strings
• Build a hardcoded static lookup table for the most common strings, e.g. the one used by default for regular event handler of popular components. This would thus be similar to the way header compression in HTTP2 has a predefined initial dictionary with common header names and values.

[pleku]

The constant pool & event data expression broken is completely at least for current Flow master version.

For a single click listener on Button or NativeButton, the key is being created after evaluating each event data expression.
So that means that it is first created for and empty json array {}, then for {"event.clientX":false}, then for {"event.clientX":false, "event.clientY":false} ...

And when a lot of event data expressions are probably less than 11 characters that the key length is, it is actually sending more data for those than what would be sent if the json string was just serialized as-is (instead of a constant key).

The good news is that is seems like a quite easy thing to fix.

[pleku]

By fixing the bug (#12124) that caused the ConstantPoolKey::calculateHash to recreate the id after reading each event data expression (like described above), the roundtrip performance for 2000 buttons with click listeners improved roughly by 40 % (on my local machine).

The difference to buttons without click listeners is still roughly x 2 (e.g. 65ms vs 130ms) and most of that comes from the additional work that is done from the server side for setting up the listeners and the event data etc.

By reusing the MessageDigest.getInstance("SHA-256"); inside the ConstantPool it only makes a difference of 10-15 % (in time) and in the end the creation of the hash is not so costly anymore but the JsonObject.toJson takes most of the time there. The following flamegraph shows the situation:

(note that the calls for new NativeButton(...) are not so costly anymore as the hashing is done much later on)

So, I think the low hanging fruit has been fixed already now (to be released to 14.7, 21+). I'm not sure if it is beneficial to try to do something else - like skipping the hashing completely for short enough event data strings. Probably the suggestion of

a hardcoded static lookup table for the most common strings

could bring the most impact, but it would also require quite much changes on the framework level.
Another alternative that comes to mind would be to improve/change the caching in other ways, so that the event data expressions would not have to be reevaluated at all (including the part for JsonObject::toJson). But that would also just affect less than 20 %.

So, I'm not sure if I should just close this issue for now or not. I'll probably still make a PR about reusing the MessageDigest as it seems rather trivial.

[Legioth]

I'll probably still make a PR about reusing the MessageDigest as it seems rather trivial.

MessageDigest is not thread safe, which means that sharing won't be completely trivial.

The alternatives for sharing that I'm aware of are:

• Have a single instance guarded by a lock - lots of contention on that lock
• Use a pool of instances - much more complex usage and a little bit of contention
• One instance per thread - leaks instances for threads that are reused
• One instance per request - maybe not so big benefit
• One instance per session - increases the size of each session

[Grelamr]

Thanks for taking a look at this!

Ill take a closer look once the next pre-release is released.

Another low-hanging fruit: JreJsonObject.stringifyOrder uses String.matches(), which compiles a lot of patterns. This pattern object could be stored in a static field.

[pleku]

MessageDigest is not thread safe, which means that sharing won't be completely trivial.

Right, good point. I just put the MessageDigest as unsafe inside ConstantPool which is tied to an UI instance which, in theory, is only accessed by one-thread-per-one-UI when the response message is being written to the client side, as that, again in theory, should only happen through the existing lock.

For more complex sharing options, again I'm not sure if the added time investment and complexity is worth it or not. Maybe it would make more sense to not use the constant pool at all for smaller json objects. So maybe best thing for now is to do nothing.

Ill take a closer look once the next pre-release is released.

It will be in Vaadin 22.0.0.beta1 and probably 21.0.4.

Another low-hanging fruit: JreJsonObject.stringifyOrder uses String.matches(), which compiles a lot of patterns. This pattern object could be stored in a static field.

Yes, but this could be handled as part of another issue. As we are using a customized version of the Elemental json library, we could change it there, but then again we have been considering of switching to Jackson for json parsing.

[pleku]

should only happen through the existing lock.

Well, should but there is no guarantee about it, so the safety of the sharing should be handled.

But anyway when I profile the code without reusing the MessageDigest and some other small cleanup for event data parsing, the part for creating the hash is about 1,81 % so I don't think that makes sense to modify it at this point. Timing wise the response takes some 15-30ms more to process (avg 130ms).

I'm going to close this issue as the reported thing has significantly improved, but in case you have any further concerns or findings, please just open a new issue and we'll take a look.

.