Onserialize improvements #302
Conversation
|
While this is good, you need to profile client as well, your patch does nothing to help the client side. |
|
Barrier pros:
#302 pros:
In my mind barriers might be a tad simpler, but that can be argued either way. |
about the 'Barrier' value: can we use something that is recognizable in hex, like 0xEE? another thing that comes to mind: -> barriers: would show error when it happens. everything after that component gets out of sync. this will cause lots of frustration if all of the sudden the monsters start looking like players. not even joking, this happened back when I first encountered this bug. we have to assume that 50% of the users will see the barriers error message and not report it. if they even see it - it's likely that more errors pop up afterwards. this would be a very stressful situation for everyone involved. users can't play after random moments, developers get negative reviews about random bugs, developers have to rush out fixes and stay up all night, etc. -> current: would show error. error might be ignored. user can still play because everything else still works, except for example the mount component. eventually the developer hears about it and can look into it without being under stress, since people can still play for the most part. imho there is a huge difference between 'after 1h of playing, everything fails' and 'after 1h of playing I can't use mounts anymore'. |
Yes, but keep in mind it makes the code more complicated too.
Yes, it is possible to do with current one too. It makes it more complicated though. Sure, the current barrier is 0xAB we can display it in hex if it makes life easier.
Perhaps we should throw and exception and abort the whole synchronization with the first barrier mismatch? Notice the random bug problem is even more likely with the current approach because of the under read problem (point 3). You may have a bug, it goes unnoticed because the current system does not report it, you go to production and people see random behavior due to corrupted reads.
I don't think trying to recover from corruption is the right approach. Ideally the entire message should be discarded and an error should be logged instead. Either way, it is possible to read corrupted data in both approaches, so there is no way to prevent this 100% |
|
under-read problem is fixed now in master. |
|
Wouldn't something like this eliminate the extra reader allocation on the client or am I missing some internal workings here that don't allow the reader to be reset? internal void OnDeserializeSafely(NetworkBehaviour comp, NetworkReader reader, bool initialState) {
int startPosition = reader.Position;
int size = reader.ReadInt32();
// call OnDeserialize with a temporary reader, so that the
// original one can't be messed with. we also wrap it in a
// try-catch block so there's no way to mess up another
// component's deserialization
try
{
comp.OnDeserialize(reader, initialState);
if (reader.Position != startPosition + size)
{
int actualRead = reader.Position - startPosition;
if (actualRead > size)
{
throw new Exception("Read too much data (should " + size + ", did " + actualRead);
}
else
{
throw new Exception("Read too little data (should " + size + ", did " + actualRead);
}
}
}
catch (Exception e)
{
reader.Position = startPosition + size;
// show a detailed error and let the user know what went wrong
Debug.LogError("OnDeserialize failed for: object=" + name + " component=" + comp.GetType() + " sceneId=" + m_SceneId + " length=" + size+ ". Possible Reasons:\n * Do " + comp.GetType() + "'s OnSerialize and OnDeserialize calls write the same amount of data(" + size +" bytes)? \n * Was there an exception in " + comp.GetType() + "'s OnSerialize/OnDeserialize code?\n * Are the server and client the exact same project?\n * Maybe this OnDeserialize call was meant for another GameObject? The sceneIds can easily get out of sync if the Hierarchy was modified only in the client OR the server. Try rebuilding both.\n\n" + e.ToString());
}
} |
|
This PR currently doesnt work for me, I get deserialization errors in a simple scene with just a NetworkTransform It seems to send a lot of 0 length packets? Is that even intended? |
|
That didnt change anything, still getting the same error |
miwarnec
force-pushed
the
onserialize_improvements
branch
from
0856177
to
047f4bf
Compare
miwarnec
force-pushed
the
onserialize_improvements
branch
from
047f4bf
to
a06677b
Compare
|
Something is not right, #302 should have no effect whatsoever on allocations in the client |
|
@vis2k according to imer benchmark, he sees: In #302 in the client he sees 2.8 MB GC and 16.60 ms In my eyes, that is significant. |
|
wasn't worried about client gc yet. I am about to test server for this one with 500 CCU again in a few minutes. |
|
480 CCU runs fine with this version. no noticeable GC spikes. we can still talk about and benchmark ondeserialize, barriers, etc. afterwards |
|
If this is working fine, merge this first and think about barriers etc. later |
|
sure, merge it, it is better than master. |
|
@imerr can you run your benchmark against 2018 branch? It has some more optimizations that should help. |
|
Server: |
|
Yep |
|
It is the exact same telepathy out of the box, that allocation should be happening in both branches. |
|
Here's current master for comparison: |
|
ahhhhhh, ok, what is happening is that allocations were previously reported in the LateUpdate, now they are reported under TelepathyTransport.Update. Those are the same allocations. If it is not too much work, might be worth redoing some of the tests. Now with the component based transports allocations will show under TelepathyTransport.update. There is also something suspicious, the 2018 branch client should be as fast as #299 or even a little faster. |
|
We should probably just merge this if we're all happy with it anyways, it's an improvement compared to master |
miwarnec
force-pushed
the
master
branch
2 times, most recently
from
8958b63
to
e690360
Compare
|
(webhook test) |
This reverts commit dbbdcd0.
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This reverts commit dbbdcd0.
### Bug Fixes * it is not safe to modify this outside this class ([bc7a961](bc7a961)) * list server logs properly when disconnected ([f02d317](f02d317)) * Make assembly definition 2018.4 compatible ([99ecc9e](99ecc9e)) * move listserver classes into package ([2668b17](2668b17)) * move NetworkStreamExtension in a namespace ([12de543](12de543)) * null reference exception ([7ce95c5](7ce95c5)) * potential null reference exception with debug logging ([33493a0](33493a0)) ### Features * Add UPM configuration ([#11](#11)) ([9280af1](9280af1)) * network writer pool to avoid expensive allocations ([3659acb](3659acb)) * update upm package if tests pass ([#21](#21)) ([7447776](7447776)) ### Performance Improvements * Optimize interest management ([f1ceb0c](f1ceb0c)) * reuse the network writer used for rpc parameters ([5dafc4d](5dafc4d)) ### Reverts * Revert "feat: Custom readers and writers" ([07ef8c9](07ef8c9)) * Revert "Onserialize improvements (#302)" ([00a3610](00a3610)), closes [#302](#302) * Revert "Documented the attributes." ([9e3dcc7](9e3dcc7)) * Revert "Documented NetworkBehaviour and NetworkIdentity." ([a5cfc81](a5cfc81)) * Revert "Documented NetworkManager." ([5bc44a9](5bc44a9))
* position magic instead of allocating writers * premature optimizations here we come * simplify. less magic. * better logging * fix ondeserialize bugs caused by readpacketuint32 instead of readint32
This reverts commit dbbdcd0.
did some more tests. I can get it down to 0.6MB GC and time closer to barriers time.
+100% safe. will never fail.
+no barrier magic
-not as fast as barrier
current:
barriers:
current-optimized:
extra: current-optimized + static writer caching:
this old UNET bug caused me so much pain, I'd really like to keep this fail safe so that a component can never touch another component's data, no matter what. or do you guys think the few ms there are worth it for barriers?