Improve message bus topic matching performance #2151
Conversation
|
|
|
Actually I am not certain if I was under the impression that it's required, because of this test in particular.However upon trying to add more test which flips all the existing tests' |
|
Hey @ryantam626 There is a chance the implementation is not correct. We only have to preserve the current functionality, and this will be revisited and optimized in Rust later (current impl here). |
|
Whoops, forgot to follow the contributor guide, will fix the CI failures. |
Polymarket has excessively long names for instruments[1], the `is_matching` O(nm) algorithm becomes prohibitively expensive when dealing with Polymarket instruments - this is further exacerbated when we maintain all instruments from Polymarket instead of a selected few. I think there is an alternative optimisation here, which involves converting the inputs into deterministic finite automata and try to walk them in sync, however this takes time to implement and validate, and thus is out of scope right now. [1] - example of Polymarket related channel/topic: `data.book.deltas.POLYMARKET.0xf6ea0a571a43a122471e1c7604d6bd8c267082a39758bdd8162dbfae0cd8d137-40739562057794550541187194569338873089092359730180442205123214215534855241164`
- Add some test involving `?` which is a wildcard we support; - Add some test involving `[seq]` and `[!seq]` (UNIX shell-style wildcard that we do NOT support);
ryantam626
force-pushed
the
rt.msgbus-is-matching-opt
branch
2 times, most recently
from
530821d to
a9f49ec
Compare
Following the previous commit for completeness.
ryantam626
force-pushed
the
rt.msgbus-is-matching-opt
branch
from
a9f49ec to
5c92022
Compare
|
Should be fixed now, sorry for the repeated force push 😓 FWIW, I think it probably is correct - I just got the wrong end of the stick about the purpose of the I will give this patch a spin and follow up with another PR if I find a better solution in the future. |
cjdsellers
changed the title
nautilus_trader.common.component.is_matching exact match performance
CodSpeed Performance ReportMerging #2151 will degrade performances by 42.43%Comparing Summary
Benchmarks breakdown
|
| cdef inline bint is_matching(str topic, str pattern): | ||
| topic_contains_wildcard = contains_wildcard(topic) |
There was a problem hiding this comment.
Small Cython thing: if a local variable isn't typed with a cdef then the Python interpreter is needed to figure out the type. Will merge like this though and optimize on develop.
Pull Request
Improve
nautilus_trader.common.component.is_matchingexact match performance as suggested in #2150.Also adds some more test cases.
Type of change
Bug fix (non-breaking change which fixes an issue)
How has this change been tested?
Existing tests are almost sufficient.
For completeness, I have also added some more tests for
?pattern which we do support[seq]and[!seq]pattern which do NOT support