Closed symphony-mariacristina closed 3 years ago
I'm wondering if this post processing is a bit too heavy for each message. A possible improvement could be to add a flag to the messageML to be set when a UIAction with target-id is found during messageML parsing. In this case we only do this post processing when we know for sure that there's a match to be found. We already do something similar for chimes https://github.com/symphonyoss/messageml-utils/blob/master/src/main/java/org/symphonyoss/symphony/messageml/elements/MessageML.java#L130 Any thoughts @symphony-youri @symphony-thibault ?
I'm wondering if this post processing is a bit too heavy for each message. A possible improvement could be to add a flag to the messageML to be set when a UIAction with target-id is found during messageML parsing. In this case we only do this post processing when we know for sure that there's a match to be found. We already do something similar for chimes https://github.com/symphonyoss/messageml-utils/blob/master/src/main/java/org/symphonyoss/symphony/messageml/elements/MessageML.java#L130 Any thoughts @symphony-youri @symphony-thibault ?
This is just about navigating a few elements in the tree right? I would say usually messages don't have many elements (like hundreds max?) so it should be pretty fast. But if you want to confirm that you can use JMH to assess the impact (with/without post processing) (MessageMLContextBenchmark)
I did a test using the JMH with a complex message and seems like the post processing is not impacting the performance of the MessageML parsing, so I guess we can keep it as it is.
do you get different results now?
Not so much fortunately :)
Results with the post processing:
Benchmark Mode Cnt Score Error Units
MessageMLContextBenchmark.parseComplexMessageMLWithEntities thrpt 25 826.608 ± 19.860 ops/s
Results without:
Benchmark Mode Cnt Score Error Units
MessageMLContextBenchmark.parseComplexMessageMLWithEntities thrpt 25 822.082 ± 20.188 ops/s
Goal of this PR is to fix some minor issues found during validation.
Closes #294