the8472 / mldht

Bittorrent Mainline DHT implementation in java
Mozilla Public License 2.0
147 stars 45 forks source link

Update RPCServer.java #32

Closed DrBrad closed 3 years ago

DrBrad commented 3 years ago

I changed the updatePublicIPConsensus - to include Big-O O(1) for faster speeds.

the8472 commented 3 years ago

I don't see how this changes the asymptotic bound it still has to iterate through 20-64 values, once that is done it currently iterates through a set that is of equal size or smaller. Granted, it's one loop instead of two, but that doesn't change it complexity-theoretic aspect.

Did this show up as significant in profiler output or something?

Also, a stylistic thing: this code base should be using mostly tabs for indentation, you shouldn't mix tab and spaces.

DrBrad commented 3 years ago

Sure its not significant, however running 2 loops for every time the DHT receives a packet will be harder on the CPU, especially for something like an android or a Raspberry Pi.

I don't mix tabs and spaces haha, must be intelli-j, kind of weird though...

Note, speed ranges from 20-24ms using your method where my method is 5-7ms.

the8472 commented 3 years ago

Note, speed ranges from 20-24ms using your method where my method is 5-7ms.

I'm skeptical, this should take nanoseconds, not milliseconds. The inner code block is only executed on responses, which is proportional to requests being issued. And when the incoming response contains an external IP that is different from the local consensus. That should be fairly rare. So in combination the code path should be very rarely taken and not show up in profiles. If it does then there must be a bug somewhere.

DrBrad commented 3 years ago

Not quite, you have it currently to update every time a node is responding, not every time the ip differs. This means that this code gets run every bucket insert not every differing ip.

I recommend atleast changing this to ensure that its not called every time we get a response:

if(originPairs.size() > 20) {

To this:

if(originPairs.size() > 20 && !addr.equals(currentExternalConsensus)) {

the8472 commented 3 years ago

Ahh, yes, that makes sense and is a good change (I mistook that change for my own code 😅). If you reduce it to that I'll happily accept it.

DrBrad commented 3 years ago

Haha alright xD