Out of world

[SafwatHalaby]

Please discuss this before merging.

This PR entirely eliminates the good old "block out of bounds" assertion failures by actually removing these assertions. Whenever a blocks with a negative y value or a too high y value is requested, it is considered an air block (because it is actually an air block) rather than asserting.
Also, non player entities immediately self destruct when their y becomes negative.

Edit: I've decided to sum up all my comments here:

• Currently, calling Getblock with a negative (or Too big) height causes an assertion failure.
• In my opinion, calling GetBlock with a negative height is not an error at all. It should simply return an air block. All heights should valid for reading (But not for writing). A pathfinder should treat a hole in the bedrock just like any other hole. In other words, treat it as air. This applies to all other parts of Cuberite that I've dealt with. So, returning air should be safe for negative heights.
• Currently, most of the GetBlock calls have guarding code: if Y < 0 || Y too big do not call GetBlock. The pathfinder even has this: if Y < 0 || Y too big do returned air; else return whatever getblock() returns;. The only purpose of all that guard code is evading an assert which, in my opinion, shouldn't have been there.

Closes #3089
Closes #3087
Closes #2539
Closes #3047
...and possibly others.

[tigerw]

The ::Get class of functions are understandably quite a hot path and used in various loops where more performance is good. The idea was to eventually remove conditional branching so there wouldn't be the need to involve branch prediction and mispredictions. Are we considering that an unreachable goal?

[SafwatHalaby]

Can boundary checking ever be safely removed?

[SafwatHalaby]

Can boundary checking ever be safely removed?

Almost all out-of-bound bugs occur at heights -1 or 256. At the cost of some more memory, some boundary checks can be removed by creating a 1 block thick sentinel at the top and bottom of each chunk, always filled with air.

[SafwatHalaby]

Is branching really a concern in this particular case? in almost all cases, a branch predictor will correctly predict this, because almost all GetBlock calls will involve a valid height.

[SafwatHalaby]

What's wrong with my new tests?

[SafwatHalaby]

This PR is also in line with the consensus that plugins should be prevented from crashing the server. This function had been a common way for plugins to crash it.

[NiLSPACE]

This looks good to me. I'm not sure why the test fails though :/

[tigerw]

Is branching really a concern in this particular case? in almost all cases, a branch predictor will correctly predict this, because almost all GetBlock calls will involve a valid height.

No clue, since nobody's done any testing.

This PR is also in line with the consensus that plugins should be prevented from crashing the server. This function had been a common way for plugins to crash it.

The checks were there before though, so it's only relaxing Debug sanity checking, right?

---

Still not wholly convinced. The amount of assertion failures here despite years of effort is testament to how badly we code makes me worry that

a) there could be bugs (remaining despite multiple 'fixes') that are not caught now that were before, and
b) duplicate checking of height validity will be harder to remove

buuut, as always, idk

[SafwatHalaby]

The checks were there before though, so it's only relaxing Debug sanity checking, right?

Sort of. The rationale here is that asking for the block at e.g. (0,-5,0) is not a logical error and there is no reason that should assert. It's simply an air block.

[SafwatHalaby]

And by insisting that it should for some reason assert, you gotta put edge-handling code all over the place. (e.g. pathfinder, entities falling to the void, etc. right now they all must make redundant careful checks to avoid this assert)

[SafwatHalaby]

there could be bugs (remaining despite multiple 'fixes') that are not caught now that were before, and

That's worth thinking about. Note that the assert for setting the blocks remains. It's as if all the blocks at Y<0 and Y>maxHeight are read-only air.

I am curious: Did this assert ever catch a legitimate bug? by "legitimate", I mean a bug in the logic. A pathfinder calling GetBlock(42,-1,30) is not a legitimate bug; returning air would make it work perfectly fine.

How about asserting for y < -100 and y > maxHeight + 100? That assert would not catch queries like (3, -1, 7), but if some code calls (142, -12951, 142), then probably something is messed up.

[SafwatHalaby]

I may have said this before but: The main idea here is that what we previously considered bugs aren't really bugs. It's perfectly fine, at least in all the parts of the code I've dealt with, to ask "What's at (15, -5, 200)". The answer would be "air".

duplicate checking of height validity will be harder to remove

The idea is not checking for height validity because all heights are valid for reading. (Or at least only checking them in the extremes to catch memory corruption bugs, as explained in the previous comment)

[SafwatHalaby]

@madmaxoft @worktycho opinion?

[worktycho]

I think this is probably a good idea. If we find a hot loop where this is actually an impact, we could then add an internal UnsafeGetBlock for intenal use, but frankly GetBlock is now complicated enough with a branch and a dereference, that its probably not going to make too much difference.

[SafwatHalaby]

@worktycho @tigerw I think we got this wrong. This change means less branches at best and the same amount of branches at worst. Why? Because we already perform boundary checks, but they're spread all around the code. Those checks would be eliminated after this PR. e.g. see the diff at #3148

[worktycho]

There are less or equal branches in the code, but more or equal on any one path. Whether that is a improvement or not is difficult to predict, because if the check is duplicated on a path, then that path could have more branch prediction stalls, but if they are not duplicated just moved, than the smaller code will result in better icache utilisation, and more focused branch prediction in a single place.

[SafwatHalaby]

@worktycho Also note that:

• We probably currently perform redundant checks (functions that have checks and call functions that have checks)
• Lua plugins currently have to perform checks. They won't have to after this. Lua is slower.

[tigerw]

...then that path could have more branch prediction stalls, but if they are not duplicated just moved, than the smaller code will result in better icache utilisation, and more focused branch prediction in a single place. ~ @worktycho 2016

^ dis guy ^ knows what he's talkin' about.

[worktycho]

If there are duplicate checks asserting conditions, then the performance can only improve, if we remove them. And getting common code out of lua is definitely an improvement.

[SafwatHalaby]

There are less or equal branches in the code, but more or equal on any one path.

Under what condition would the new code spawn more branches on one path? The way I see it is that we're merely moving the checks from everywhere to GetBlock, so the new code should either have an equal amount of branches, or less (In the case of redundancy, functions performing checks and then calling functions that perform checks).

[worktycho]

The condition it would produce more is if you have missed removing a redundent check somewhere.

[SafwatHalaby]

Merge?

[worktycho]

I say merge.