Weather, Storms & Tornadoes

Weather, Storms & Tornadoes

11M Downloads

[1.7.10] Too much network traffic, you're doing something wrong...

cosmicdan opened this issue · 11 comments

commented

Sorry to sound rude, but this is really shocking:

clipboard01

How is this even possible? Are you doing rendering logic on the server side or something, and sending complex graph data to each client (this network traffic is with only one player online, when a second comes on the traffic doubles, etc.)?

I don't know if you have any interest in updating 1.7.10 anymore, if you don't can I get permission to try and improve this myself (and use the fork in a modpack we're working on)?

commented

Yeah it's always been pretty terribly optimized, still is in 1.10.2 as well, a lot of redundant data being sent.

I would welcome a PR for optimizations.

For storms that are less than high wind intensity, it sends a sync packet every 40 ticks, for storms high wind or greater, it sends a packet every 2 ticks.

This is the code it sends every x ticks, kinda a lot: https://github.com/Corosauce/weather2/blob/master/src/main/java/weather2/weathersystem/storm/StormObject.java#L280-L366

This is the code that decides rate and sends out packet: https://github.com/Corosauce/weather2/blob/master/src/main/java/weather2/weathersystem/WeatherManagerServer.java#L78-L83

in 1.10.2 things have been slightly refactored to a base weather object class for storms and sandstorms to extend, also a method added for the object itself to determine update rate. Not too much has changed for 1.10.2, so I might be able to migrate whatever changes you make to 1.10.2.

for the calm storm objects, really the only values that change regularly are probably pos and precipitationAmount.

I did have plans to optimize this way back, to bring over the entities datawatcher system to my weather objects, but that never happened. The 1.10.2 entity datamanager system is even nicer now too, would make it a lot less of a pain, shame that version isn't in 1.7.10.

Good luck!

commented

Good stuff, thanks! It might be a while though, we're working on a modpack and there's a lot of other custom stuff I need to implement first - but this is definitely high on my priority list (server latency is an issue for us Australians).

commented

Oh. This "entities datawatcher system" you speak of - this is something you wrote for the 1.10.2 update, right? Not a Forge for 1.10.2 thing? I'm still yet to explore, fiddle and learn how your mod does it's thing, but I'm curious if it'd be worth my time to learn that and back-port it.

Btw, we are using the global overcast on our server (is that the one...? whichever option means "rain happens everywhere at once ala vanilla"). In that event, could any of the traffic be considered unnecessary overhead (if you remember)?

commented

Off the top of my head the global overcast option shouldn't affect network load at all.

As for the datawatcher system, its a new improved vanilla feature in 1.10.2. Though looking at it in more detail now it seems to be designed specifically for entities, so my earlier suggestion of using it wouldn't work without copying and redesigning it for more generic purposes, specifically the EntityDataManager class and network code related to it.

Still a nice change from what we used to have. Check out EntityZombie class for example, create a dataparameter with:

private static final DataParameter IS_CHILD = EntityDataManager.createKey(EntityZombie.class, DataSerializers.BOOLEAN);

register it with: this.getDataManager().register(IS_CHILD, Boolean.valueOf(false));

set it with: this.getDataManager().set(IS_CHILD, Boolean.valueOf(childZombie));

and the client automatically gets the latest value when its set, and can get that value with: ((Boolean)this.getDataManager().get(IS_CHILD)).booleanValue();

Still work migrating over for non entity syncing I say!

commented

OK, I I've improved it a bit by coding in some stuff to make it only send changed values in the packet. The numbers are a bit better (for the same packet count, anyway):

tmp

It's still a bit high though, unusual since I'm only sending ~100 bits per packet on average. Maybe it's mostly overhead caused from sending an individual packet for each storm? Maybe I'll try combining them all into a single packet next, but that will probably take a bit of work.

Well, here is the relevant commit if you want to have a gander at the work so far - https://github.com/HostileNetworks/weather2/commit/372150c87f56ea1619fa8cc0199a99ae43a26f4f

commented

Ok, my hunch was right - the overhead involved in sending individual packets for each storm is huge. I wrapped them all into one packet (just nested the NBTTagCompound objects) and it's now down to a constant <1KB/sec. You can check the same branch I linked above.

Pretty happy with those results. Will play test for a while before sending a PR, but initial testing seems fine to me.

commented

Well backporting that whole system might be a bit overkill haha, but I think I get what you mean. It sounds like a packet handler/wrapper that only sends payloads containing values that have actually been updated since the last payload. I suppose I could do a similar thing for the NBT tags you pointed me to, albeit a rudimentary implementation. I'll see if I can figure it out.

Sending a packet every 2/40 ticks seems a bit too high to me, but I suppose for singleplayer it's alright. Best choice there is to offload those two values to config. In fact I will offload all of those "tick rate" numbers to a config, because why not? Will let the creator of the modpack I'm working on play with the values without pestering me :)

I'm also replacing all those modulo's with some counters, division makes me cringe (not many people know that a div operation is often 10x slower than an add operation!)

commented
commented

Now imagine how much traffic when many mods sends 1 change - 1 package, up to 4GB per hour in not built-up world is the norm))

commented

Nice, I like the implementation, no complaints from a first look at it, I'll have a look at the PR and test it out once you make it.

commented

PR for 1.7.10 merged in, and I also adapted the strategy to 1.10.2 version.