fix: fdcostly should take only the prefix into account #5
Conversation
filter.go
Outdated
| if len(mas) < 2 { | ||
| return false | ||
| } | ||
| a = mas[0].Encapsulate(mas[1]) |
There was a problem hiding this comment.
this seems pretty memory expensive...
There was a problem hiding this comment.
better idea how to do it?
There was a problem hiding this comment.
A single a.Protocols() and then a check whether any of them is P_TCP, should simplify this a lot. Then mafmt.TCP.matches(a) can be dropped, which actually does quite a lot under the hood. Collecting only the protocols is less work than building up the whole thing and then matching. (We should amend multiaddr-fmt to the same, but that's for another time.)
That would also make this function more future-proof, TCP won't neccessarily always be at position 2 in the address, but it'll always be FD-costly.
|
@Kubuxu is this patch still relevant? |
also add tests for it License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
Kubuxu
force-pushed
the
fix/fdcostly/prefixx
branch
from
f54ed6e
to
83e4765
Compare
ghost
assigned 
|
I've rebased it and it is relevant. Currently |
filter.go
Outdated
| if len(mas) < 2 { | ||
| return false | ||
| } | ||
| a = mas[0].Encapsulate(mas[1]) |
There was a problem hiding this comment.
A single a.Protocols() and then a check whether any of them is P_TCP, should simplify this a lot. Then mafmt.TCP.matches(a) can be dropped, which actually does quite a lot under the hood. Collecting only the protocols is less work than building up the whole thing and then matching. (We should amend multiaddr-fmt to the same, but that's for another time.)
That would also make this function more future-proof, TCP won't neccessarily always be at position 2 in the address, but it'll always be FD-costly.
ghost
self-requested a review
License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
|
I've imported |
Good point -- maybe use |
License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
|
Leeet's go! Thanks 👍 |
also add tests for it