Conversation
|
Your email adress is incorrect, you'll need to give @Zbob750 the real one so he can correct it. However, I wonder why we need to 'predefine' the antixray blocks in the palette, can't we just add them to the palette as we go? |
| + private final boolean asynchronous; | ||
| + private final int neighborsMode; | ||
| + // Used to keep track of which blocks to obfuscate | ||
| + private final boolean[] obfuscateBlocks = new boolean[Short.MAX_VALUE]; |
There was a problem hiding this comment.
Quick look over the code tells me this is iterated over. Switch to using bitfields in a long array to better-utilize the CPU cache.
There was a problem hiding this comment.
That is a totally unnecessary optimization.
|
Email: minecraft.stonar96@gmail.com Predefined blocks: The size of a DataPalette is limited, which means that it would be sometimes necessary to create a completely new DataPalette while obfuscating for the blocks which are used. That's not possible while obfuscating because at this stage the DataPalette has already been written to the packet. It would be possible to add the blocks before the packet is created but the most efficient way is to add them from the beginning, so that recreating the Palette is never necessary. This was actually the reason why Anti-Xray was removed from Spigot in 1.9. md_5 wrote that they don't find a way to implement it efficiently if the DataPalette changes. Snapshot or Copy: The packet also requires lightning and biome index. It would be also necessary to copy the blocks of neighbor chunks (copy blocks of 5 chunks + lightning and biome index of one 1 chunk per obfuscated chunk). That's in fact much. Edit: Note that it would be more efficient to copy the whole DataBits array of the neighbor chunks than creating a ChunkSnapshot of the neighbors with the edge blocks. The lock is still the "cheapest" way for RAM and CPU. Deadlocks: Deadlocks are not possible and they are easy to avoid with my system. lock() and waitUntilUnlock() are on the same thread and only on the main-thread. lock() does not block or wait at all and unlock() is called on the obfuscator thread which is never locked. And it wont break in the future versions unless mojang adds much concurrency. Updating is easy: Just call waitUntilUnlock() before anything which is required to create the packet is changed. There wont be a risk for deadlocks or race conditions. This is currently the most efficient Anti-Xray implementation and that's the reason why poeple like it. And my goal was to make it as efficient as possible. I can see that most of your concerns are about the asynchronous part but we can also just remove it completely and all problems are solved. It is still very efficient (more efficient than Anti-Xray before 1.9). |
| + chunk.blocksLock.lock(); | ||
| + chunk.dataLock.lock(); | ||
| + | ||
| + if (nearbyChunks != null) { |
There was a problem hiding this comment.
Why mix of TAB and spaces to indent
There was a problem hiding this comment.
I don't know how that happened. I think I used another editor. I will fix that later.
|
I need more time to review, but i'm not really fond on async being configurable. Async should work w/o issue, and if it does, then it shouldn't be an option to not use it. I rather us provide a consistent experience that doesn't overload the user with unnecessary and opinionated options. |
|
|
||
| - this.a(networkmanager_queuedpacket.a, networkmanager_queuedpacket.b); | ||
| + if (networkmanager_queuedpacket.a instanceof PacketPlayOutMapChunk && !((PacketPlayOutMapChunk) networkmanager_queuedpacket.a).isReady()) { // Paper - Async-Anti-Xray - Return false if the queue contains a not ready chunk packet | ||
| + return false; // Paper - Aync-Anti-Xray |
There was a problem hiding this comment.
Why is a packet that is not ready to be sent queued in the network manager?
Why isn't a design in-place where the packet is only queued IFF it's ready to be sent?
There was a problem hiding this comment.
It's not that easy as you might think to just queue only the chunk packet and send it later. The order of the packets is sometimes important (eg chunk packet then block updates). That's the reason why I don't call sendPacket(packetPlayOutMapChunk) later or asynchronously when the packet is finished. It is still called synchronous (with the not finished packet), so that the server "knows" that it can now send blockupdates for example. That's reason why all packets are queued in the right order, finished or not.
NMS code from PlayerChunk:
public boolean b() {
...
PacketPlayOutMapChunk packetplayoutmapchunk = new PacketPlayOutMapChunk(this.chunk, '\uffff'); // Create the packet
...
entityplayer.playerConnection.sendPacket(packetplayoutmapchunk); // Call sendPacket with the not finished packet ...
...
return true; // ... because we return true here which "tells the server: Now you can send block updates or whatever"
...
}
There was a problem hiding this comment.
What we could do if a packet does not deal with chunks or blocks such as chat or movement ... is to make it bypass the queue but I don't think that makes a difference.
There was a problem hiding this comment.
Yes, but that does come at a price - maintaining this for the future. If such a system was it place, it would heavily modify the packet system, which may or may not be maintainable.
|
@aikar That's fine. Please tell me what exactly I should remove when you have finished reviewing. |
|
You could use co.aikar.timings.MinecraftTimings.antiXrayUpdateTimer and .antiXrayObfuscateTimer. |
| + public int neighborsMode; | ||
| + public AntiXray antiXrayInstance; | ||
| + private void antiXray() { | ||
| + antiXray = getBoolean("anti-xray.enabled", false); |
There was a problem hiding this comment.
If anti-xray is not enabled, can the rest of the code in this method be skipped?
There was a problem hiding this comment.
No we need to add the defaults and stuff
|
Any update to this? :D I'd love to have a lightweight, simple, built-in anti-xray function again. |
|
We have a custom build with this patch running on our servers for about 2 weeks, because we really needed anti-xray and it works very good. would be great if this can be included into paper. |
|
Running asynchronously? That is probably the biggest hurdle at the moment. If it runs well and is threadsafe async should be the only option, if it doesn't it should either be fixed or removed. As covered nearly a month ago. |
|
Yes we have asynchronous enabled |
|
@Zbob750 I am still waiting for the response of @aikar. There are more options than just asynchronous-mode and he said that he doesn't want that. |
|
I think most options are useful and should not be removed because it is mostly quality (of ore hiding) vs performance. Some may want better anti xray and some others just want it faster.. our current configuration (fast but not perfect hiding of ores): |
|
@stonar96 @Brokkonaut @kashike @aikar code review when you get the chance please. Edit: if it is decided to be done synchronously, it'll need timings sections added. |
|
@Zbob750 Yes, I can understand that there shouldn't be an option for that. I am very sure that both synchronous mode as well as asynchronous mode work fine without issues (thread safe without deadlocks or inconsistencies). I will wait for the response of @aikar if I should remove asynchronous mode or keep asynchronous mode but not configurable. Even in asynchronous mode we should add timings for the locks on the main thread and for the update method. Edit: In synchronous mode: The reason why I didn't add timings is that the obfuscator code also contains the default work (writing to the packet). So the timings would show obfuscation time + default packet creation time. Should we just ignore that? |
|
We recently had this crash on one of our servers: http://pastebin.com/raw/g7PbV4dt I think it is connected to anti xray because it happend in this line: I think a check if |
|
@Brokkonaut thanks for the report. We can add a null check here but the actual question is: Why is there a null packet in the queue? The exception would also occur in the vanilla code because there is also no null check before accessing the field a. So this is rather a vanilla bug (which would not occur in vanilla because the queue is usually not used) which we should fix here. It's not a bug of Anti-Xray. |
|
I'm not sure about the reason. The only thing I can say for sure is that we had this crash. One idea is, that between the !i.isEmpty() call and the i.peek() call the queue was cleared (for example in handleDisconnection() it is cleared - I don't know if that is called async) - another idea would be some conflict with some plugin. We use ProtocolSupport and ProtocolLib (For NCP, Citizens and LibsDisguises). |
|
@Brokkonaut True, actually there should be a lock inside the handleDisconnection method when the queue is cleared. I will just fix it with a null check. Thanks. |
|
Removed two comments here for being off topic. If you don't have a code quality or other development focused reason to post here, please don't. We get it, you want your anti-xray, but we're not merging it until it's ready. Feel free to compile it yourself and test for us, but "+1" and "pls merge" comments will be removed. EDIT: The original issues are still present, is this going to be async? Where are the timings sections? If needed we can look into this ourselves, but I'm hesitant to just take over someone's PR. |
|
I provided feedback to stonar over IRC and to my knowledge he should be working on those. To anyone else... be patient he's working on it (I hope) |
… to BlockPacketController#createPacket(...)
|
In the latest commit I removed the obfuscation methods from the DataBits class because I don't think that they should be there. Instead I have added getters for the required data to move the whole logic to the BlockPacketControllerObfuscate class. Thereby I was also able to reduce the amount of calculations and I think the code is more clear to understand now because you can see directly what's happening. However, if you disagree and you think that the obfuscation methods in the DataBits class were better I will remove this commit. |
|
I thought that @Techcable removed DataBits? |
|
that is not merged, yet.
|
aikar
left a comment
aikar
left a comment
There was a problem hiding this comment.
Sorry I didn't catch this sooner, but I'm really concerned about the locks as I just discovered the waitUntilUnlock's are occurring on the main thread, when I thought they were happening async.
This highly concerns me from a performance aspect, as you will be blocking main thread in multiple places.
| } | ||
|
|
||
| - protected IBlockData a(int i) { | ||
| + public IBlockData a(int i) { // Paper - Anti-Xray - protected -> public (Used inside the obfuscator loop) |
There was a problem hiding this comment.
This is extremely important to be an obfuscation helper
There was a problem hiding this comment.
Thanks, I will add it.
| } | ||
|
|
||
| public void setType(int i, int j, int k, IBlockData iblockdata) { | ||
| + // Paper start - Async-Anti-Xray - Lock the blocks |
There was a problem hiding this comment.
All these locks in this class scare me. I was under the impression that the waiting would be done async? I'm worried this code is going to drastically slow down mass updates / cause lag.
These methods will be called on the main thread.
Note that there is a "dirty" count, that after 64 changes, the chunk is resent to client.
So I can see mass updates triggering these locks multiple times as the packets are sent.
|
could we potentially move the locking to be done at the end of the tick, and then process chunk sends then to give the most absolute shortest tick? Id prefer if we could avoid modifying those methods at all, and basically make chunk sends wait until the server is not ticking/end of tick |
It is definitely required to wait on the main thread. The data of the chunk should not be changed aslong as the chunk packet is created because of thread safety. But that's not the only reason. We have to ensure that everthing happens in the right order. For example if a chunk packet followed by a block update should be sent to a player it is necessary to send the chunk packet first otherwise the block update will get lost. That's the reason why I invoke Lock#lock() on the main thread when the server attempts to send a chunk packet. In other words, when the server attempts to create a chunk packet the data of the chunk should not be changed, not only to ensure thread safety but also because of the state (or order) of the data which is sent. That means if the server attempts to create a chunk packet it must be created immediately (with the right state) which makes it necessary to wait on the main thread if the server tries to modify data of the chunk. I can't understand your concerns about the performance because it is still faster than doing it sync on the main thread because the main thread is then blocked for the entire obfuscation time. At least the main thread can move on working with other stuff until it tries to modify such data which doesn't happen always at all and as I have already said before this Anti-Xray implementation isn't even slow in synchronous mode. However, there is a solution without locks using chunk snapshots or a copy of the whole chunk which is created on the main thread and used async to create the packet but this is what I have tried to avoid and it won't be more efficient than the locks.
I don't know when chunk packets are actually created and sent. I think it would be possible to do it at the end of a tick but isn't it actually quite at the end?
Assuming you mean doing it still async: The locks would be still necessary anywhere at the beginning of a tick or in these methods then to handle cases where the tick is to short or the obfuscation takes to long and if there is no time remaining for obfuscation it would be kind of sync anyway then. All in all my opinion is that the current implementation is the most efficient one in relation to the effort and as I have already said a few months ago we can also just make it sync at all and all problems are solved (which is a contradiction because then it would be less efficient). |
|
Spigots anti xray abused "relatively safe" async operations in the past. I think I'm more comfortable with accessing the data async w/o locks, with the risk of changing more than the locks. Worst case scenario would be a block changing in a chunk that would either cover or uncover an obfuscated block, leaving it without a perfect result. However, the likelihood of this happening is sooooooo rare, as the player is guaranteed not going to be modifying a chunk that they are receiving, and the likelyhood of another player modifying the chunk as its being sent to another client in a way that actually impacts the receiving player is also rare. If an ore was uncovered, the uncovering player is likely going to collect it themselves. But we are talking such extremely rare chance of this scenario even occuring. @Zbob750 thoughts? If it became a problem, we could work to resolve it then. |
|
Wrong, worst case scenario is a server crash incase of blocks are placed which were not yet in the data palette (which didn't exist in the past) and even most likely when lighting updates occur durring this process because the packet size is calculated before the packet is created. Therefore we have to create the packet with the same data which was used to calculate the size of the packet. To ensure that we need the locks. There is also a difference in my implementation: The obfuscation is part of the creation of the chunk packet and (almost) the whole chunk packet is created async. Orebfuscator and the old Spigot Anti-Xray modified the packet after it was created sync. |
|
...why does any of this need to be done on the main thread? Simply "take over" the packet in PlayerConnection#sendPacket. You can insert an "if main thread and instance of PacketPlayOutChunk", put those packets into a CLQ and modify those async. You can then send those modified packets - all with almost 0 hit to main thread operations. |
|
|
The packet order would be maintained through the use of the CLQ. DataBits will likely removed in an upcoming commit (has been a PR of Techcable's for a long time) Why do you have to access edge blocks of neighbour chunks? If this is 100% requires you could simply do it with a Future and run it on the main thread. It may be more "efficient" but would make the main thread tick slower. I would rather my ticks be faster and then send packets after. PlayerConnection#sendPacket is thread safe anyway so would work perfectly. |
|
As this is stable and working I would recommend merging it as it is and just adding some timings to see how much time those locks will take on the main thread. if it is really too much it is possible to look for some optimisation after the merge. As you know we have this anti xray on our servers for some months and could not see much performance impact. But even if it has - imo slow anti-xray is better than no anti-xray and if someone does not want it he does not have to enable it at all. |
My guess is when you're mining near/at chunk border |
|
to know if a block at the border of a chunk should be obfuscated you need to know if the adjacent block in the next chunk is intransparent or not. |
|
@AlfieC There are also other packets which deal with block updates (As I said multiple times before: first chunk packet then block update). I already use a queue to solve that problem and basacially I already do what you said (except for taking the finished packet and modify it async, I create and obfuscate the packet async at once which has the drawback that I also need a lock for lighting not only for the blocks). I just wanted to explain why there is still code to execute on the main thread.
The lock implementation I use is basically the same as Future#get() on the main thread (Lock#waitUntilUnlock() in my case). I invoke it on the main thread before such data like edge blocks of neighbor chunks are modified.
As far as I know a DataBits instance is created when the chunk is sent.
Of course it is thread safe but that's exactly what I meant with the packet order. If you call sendPacket async after the chunk packet is finished you can't ensure the right order of the packets. That's why I invoke sendPacket sync on the main thread with the not finished chunk packet and the queue in the NetworkManager takes care about the packet order (also other packets). And now please stop. If someone thinks to know it better please try to create your own implementation. |
|
The DataBits PR wont be merged unless strong justification showing improvements can be given. Not enough evidence has been given to prove its worthwhileness on such a maintenance burden patch. |
This is not acceptable behavior. Discussion around the idea of avoiding the locks is highly desired. it's not a good idea to "shut down" discussion on how to improve the changes to resolve concerns we have with the patch. To help you understand my concern -- the locking mechanism invokes lower level calls. This is a highly disruptive execution pause, that can drastically affect performance and invalidates CPU branch prediction. even if its not locked, the mere existence of this code causes the jvm to have to reconsider code flow and predictions. Branch Prediction is a huge part of performance, and what once was always a predictable pipeline, will now become unpredictable due to this change.... We will have to think on this, as the mere existence of this code, even with anti xray turned off, can impact others server performance, which is not acceptable. |
|
How about adding timings to the implementation? So it would be possible to test how much time is used every tick for the locking in the main thread on a real server instead of speculating how much it could be. As I said we use this patch for some time now on our servers, because we needed anti-xray - if there would be timing data I could share it. |
|
@aikar
There are several ways but all of them have also disadvantages.
I have already added no operation implementations.
This discussion won't help me and won't improve anything. I can't see a valid argument or suggestion. He is just saying how simple it is without knowing anything about the current implementation (e.g. what the locks actually do) and how it works and it's just annoying to repeat things which I have already said and explained over and over again. |
|
In the current implementation the locks are necessary, however it also has some advantages. The reason why the locks are necessary is because the packet is also created async, not only obfuscated. Without the locks it is possible that the whole packet gets invalid or an exception occurs if there is suddenly more data to write than expected. Good news: I am currently working on a new implementation without locks (at least without data lock, the blocks lock is optional if the blocks are only used to check if a block is hidden). I will create a new PR for the new implementation and additionally I will close this PR and also create a new one for this (of course I will add a link to this PR). The reason is that I don't want these commits on the master branch of my Paper fork anymore. |
17 participants
Paper inbuilt Anti-Xray. Asynchronous mode is configurable.