Replace xml_stream with exml #183
Conversation
|
Looks good to me, though I didn't test the changes. |
|
Shouldn't be merged yet, I'm doing load tests first. :) |
| gen_fsm:send_event(C2SPid, {xmlstreamerror, "XML stanza is too big"}); | ||
| true -> | ||
| if | ||
| element(1, Elem) == xmlel -> |
There was a problem hiding this comment.
I think that pattern matching in a case like following would be more readable (in my opinion)
case Elem of
#xmlel{} ->
gen_fsm:send_event(C2SPid, {xmlstreamelement, Elem});
_ ->
gen_fsm:send_event(C2SPid, Elem)
endThere was a problem hiding this comment.
Indeed. And it involves including exml.hrl, so to maintain consistency all element(1, X) in ejabberd_receiver should be replaced.
|
This pull request refers to #178 |
|
A tip: let's put issue number in the commit message (if issue exists of course) |
|
Any diagrams you can put here to prove that exml is more stable/efficient? I know you have something, don't hide it :) BTW, please rebase to master :) |
|
I want to double-check if it was valid result. I was experimenting with various VM versions (for benchmarking and segfault debugging) and I might have made some mistake. :P |
|
I must have done something wrong last time. Final tests reveal that there is no difference in terms of performance. The only benefit is cleaner source code (C modules handling XML kept in one application). Will we even merge it after rebase? |
|
I know it's been a long time but we finally came into conclusion that this change is worth merging. Could you rebase it or make it "green" for merge in other way? |
|
OK, will do it during some pause in commercial work. Should be quite soon actually. |
|
on it |
|
@pzel, any progress? |
|
I spent some time analyzing the security implications (cdata flood & c2s shaper functionality) and I'm not certain we can proceed without tests and security analysis. I'll file an issue with esl/exml. |
|
Until this serious exml bug is fixed, we should put the project on hold. |
ppikula
force-pushed
the
exml-v2
branch
2 times, most recently
from
e245d15 to
68c678a
Compare
|
Fixes #414 |
ppikula
force-pushed
the
exml-v2
branch
5 times, most recently
from
153a240 to
7a4ea6d
Compare
|
Performance comparison against master branch: https://snapshot.raintank.io/dashboard/snapshot/prLZfftfcq2XhSflK8LcbyiqZZ0tITey First run - master, |
the problem was with the startls tests, they call the starttls function without calling become_controller, parser is not created and recever crashes
|
Sorry for the delay. without exml: with exml (with refc binaries): with exml (no refc binaries): |
Replace xml_stream with exml
This is a reincarnation of Rafał's
exmlbranch. The original was too difficult for rebase/merge with current master. :( We can compare performance of modernexmland MongooseIM withxml_stream-based version. Perhaps with this we could avoid virtually duplicating functionality in another C module?