Conversation
yehonatanz
left a comment
yehonatanz
left a comment
There was a problem hiding this comment.
The core concept seems good, I do however have some notes regarding the implementation.
tomlegkov
left a comment
tomlegkov
left a comment
There was a problem hiding this comment.
Nice implementation, I like it 👍
I'm mainly concerned about hits to performance
|
|
||
| class Marine: | ||
| SUGGESTED_MACROS = { | ||
| "macro.ip.src": ["ip.src", "arp.src.proto_ipv4"], |
There was a problem hiding this comment.
It's not a part of the macro because it isn't in the current version of marine.
| self._marine.destroy_marine() | ||
|
|
||
| @classmethod | ||
| def _expand_macros( |
There was a problem hiding this comment.
Did you analyze the performance of the macros? I recall it slowing down Marine quite significantly.
Since the macros field is usually the same (or from a basically closed set), a caching mechanism can speed this up nicely :)
There was a problem hiding this comment.
It does slow the performance of marine.
More percisely, it slows from around 15,000 pps to 12,500 pps.
I tried implementing a caching mechanism but that did not help the performance. I think this is due to the fact that we can only cache the action performed by _expand_macros, while _collapse_macros is performed per result.
There was a problem hiding this comment.
Doesn't seem too bad for me (but sucks of course).
Remove the caching mechanism if it doesn't help.
shaharyarn
force-pushed
the
feature/macros
branch
from
35f627d to
65cc141
Compare
| self._marine.destroy_marine() | ||
|
|
||
| @classmethod | ||
| def _expand_macros( |
There was a problem hiding this comment.
Doesn't seem too bad for me (but sucks of course).
Remove the caching mechanism if it doesn't help.
shaharyarn
force-pushed
the
feature/macros
branch
from
796c9ea to
4bcae36
Compare
yehonatanz
left a comment
yehonatanz
left a comment
There was a problem hiding this comment.
I had some notes, but my main worry is that the macros_indices API is too complicated, and therefore makes your client code too complicated.
See my note on the C code here: tomlegkov/marine-core#35 (review)
yehonatanz
left a comment
yehonatanz
left a comment
There was a problem hiding this comment.
Had a tiny note, but this looks good!
3 participants
Linked core PR: tomlegkov/marine-core#35