search

CylonSB / bounded-planet

Test Game Please Ignore
6 stars 1 forks source link

fix/arc_nonsense #72

Closed martindevans closed 2 years ago

martindevans commented 3 years ago

Previously the network system handled all packets as Arc<Packet>. This was unnecessary for many packets (cloning is cheap), slow (Arcs are expensive) and a pain to work with (e.g. matching through Arc). Removed that, packets can internally use Arc where they contain expensive to clone things (e.g. WorldTileData now contains an Arc<MeshData>).

Packets were previously delivered inside a ReceiveEvent, which every system had to filter to a packet event, and then had to filter again to packets that system cared about. This was horrible to write (and also slow, lots of unnecessary filtering). This PR adds dispatching of packets out to a per-packet-type event, which systems can subscribe to.

martindevans commented 3 years ago

The caveats of Serde+Arc aren't particularly relevant to packet serialisation - since a packet will never be serialised and then deserialised on the same machine it's natural to assume a copy will be made (after all: what else could happen).

Jerald commented 3 years ago

My only thought on the larger direction of this is that now there is an additional concern when making new packets: the size and/or cost of cloning.

Previously you just made your packet be whatever it is you want them to be, and you didn't have to worry about accidentally slowing stuff down with large types and/or expensive clone implementations. Now, this is left up to the creator of the packet type, creating an opaque design footgun that's not obvious.

While in some ways I do see how this is "cleaner" and less stupid, I also think that it hid a lot of annoying design complexity that now needs to be addressed once again.

martindevans commented 3 years ago

Unfortunately I think that's just something we have to deal with. Throwing an Arc around for every single packet really wasn't a very good idea!

Also of course to some extent you have to be thinking about the cost of cloning anyway. Since it's a packet that gets sent over the wire (which is a similar cost to cloning).

ocornoc commented 3 years ago

To be clear, you profiled this, right? This is an issue with any significant impact?

martindevans commented 3 years ago

No and probably not.

Jerald commented 3 years ago

No matter how we cut it, packets will need to live somewhere on the heap to outlast the individual systems they're from. Whether it's an Arc or some internal bevy magic is irrelevant, the cost is already paid. Because of this, I think it's not going to be such a concern that Arcs are going to actually limit is for real.

Basically, I think the advantages outweigh the possible disadvantages. As some smart computer scientist said sometime: "premature optimization is the root of all evil"