[Queue approach] Fix entity destruction in unticking chunks #2986

SafwatHalaby · 2016-02-07T18:25:48Z

Closes #2731
Closes #2973

@tigerw Is that the vision you had in mind?

SafwatHalaby · 2016-02-07T18:26:33Z

I had to have a single new entity variable: m_ParentChunk. Because the parentChunk is needed for efficient queueing.

SafwatHalaby · 2016-02-07T18:30:50Z

This can be used as a basis for a problem @madmaxoft once mentioned: Entities may tick twice when moving between two chunks. But I want the basic thing to be merged first.

tigerw · 2016-02-07T19:48:15Z

src/ClientHandle.cpp

@@ -129,7 +129,6 @@ cClientHandle::~cClientHandle()
 			World->RemovePlayer(m_Player, true);  // Must be called before cPlayer::Destroy() as otherwise cChunk tries to delete the player, and then we do it again
 			m_Player->Destroy();
 		}
-		delete m_Player;


You sure about this? In the current system, the ClientHandle manages the player pointer's life.

Well, no, I am not sure. m_Player->Destroy(); will add the player destruction to the queue. The problem I can see is that m_Player->Destroy(); is inside a conditional "if" so it's not guaranteed to free it. Guess I'll change this and make the player destruct here as usual.

tigerw · 2016-02-07T19:48:55Z

Could you not use DoWithChunk() as you have done before, without storing a parent chunk?

tigerw · 2016-02-07T19:52:17Z

src/World.h

+		bool m_FreeMemory;
+	};
+
+	std::list<EntityRemovalQueueItem> m_EntityRemovalQueue;


cWorld already has a task system, I was thinking of something like:

cWorld::QueueTask([...](...) { DoWithChunk(..., ..., [...](...) { a_Chunk->RemoveEntity(); // New function to be added; delete if not player } }

Is the task guaranteed to be executed just after the mob tick and before the next tick cycle?

I just checked. The tasks are done before the mobs are ticked.

Oh yeah, mobs are ticked separately (ugh). In that case, move the TickMobs call to after m_ChunkMap->Tick, and now its guaranteed :)

TickMobs is already after m_ChunkMap->Tick. How does that help?

As in directly afterwards/before TickQueuedTasks.

SafwatHalaby · 2016-02-07T20:02:02Z

Could you not use DoWithChunk()

@tigerw That technique was dangerous. It returns the chunk the player is currently standing at. At the start of each tick this is exactly the same as the parent chunk, but this will change if the entity crosses a border mid-tick, (and will re-converge to the same chunk when cChunk::tick calls MoveEntityToNewChunk.) So, if a player teleports while walking, it may do nasty things.

The only thing guaranteed to do it right is the parent chunk (the curretly owning chunk). I am actually considering removing the a_Chunk parameter in the cEntity::tick method and its overrides, as this is always equal to m_ParentChunk. Note that using a_Chunk directly is impossible, because sometimes teleportation /destruction is called outside of a tick and therefore a_Chunk is not available.

SafwatHalaby · 2016-02-07T20:05:53Z

Hmm, There's a chance I'm wrong in my last post. I'll need to re-check this.

SafwatHalaby · 2016-02-07T20:26:31Z

Not wrong.

SafwatHalaby · 2016-02-07T20:27:21Z

Also, regardless of my last argument, I think it'd be wise to have the chunk stored for convenience, rather than passing around a_Chunk all over the place.

tigerw · 2016-02-07T20:42:19Z

Not wrong.

Are you certain? GetChunkX/Z() returns the current chunk based on the entity's position, DoWithChunk gets that chunk. The task will execute after Tick (after our fix); and Tick will have ensured that the entity is in the chunk that its position says it is in.

tigerw · 2016-02-07T20:43:14Z

(this is all so confusing, I've already forgotten why this whole thing is needed about four times, and had to re-examine your explanation of it)

SafwatHalaby · 2016-02-07T20:46:50Z

Are you certain? GetChunkX/Z() returns the current chunk based on the entity's position, DoWithChunk gets that chunk. The task will execute after Tick (after our fix); and Tick will have ensured that the entity is in the chunk that its position says it is in.

Oh, yes, you're correct, I forgot that this will happen after the tick now that there's a fix. Confusing indeed!

But my guts tell me this is dangerous futurwise. It's the Cuberite equavilant of tinkering with undefined compiler behavior. Any future change may break this guarantee. m_ParentChunk seems a lot safer to me.

tigerw · 2016-02-07T20:50:51Z

But then again, some future change may accidentally try to update ParentChunk instead of comparing it or some future chunk refactor may completely miss the fact that there's an additional variable which needs maintenance.

I personally would do the "as little amount of variables as possible" route but I think both have pitfalls, so you decide.

SafwatHalaby · 2016-02-07T20:55:35Z

Well, I've explicitly mentioned in the docs that only cChunk::addEntity has the right to update it.
I can't deny there are still dangers, but I think the dangers of your proposals are more real. For instance, the code that moves entities to neighboring chunks might one day be offloaded to the world task system to solve @madmaxoft 's double-tick issue. This will instantly break the following guarantee: and Tick will have ensured that the entity is in the chunk that its position says it is in., there are probably a million more ways this could break when doing seemingly unrelated changes. On the other hand, the only way m_ParentChunk could only break if someone sets it explicitly which is a much more verbose, much less sneaky error.

tigerw · 2016-02-07T20:56:36Z

Very well, go for it then :)

SafwatHalaby · 2016-02-07T20:58:41Z

One more question first: Should I really go through the cWorld's existing task system? How will it fare performance-wise? Perhaps it'd be wise to have a second, entity-specific task system instead? Entities will produce a lot of tasks if chunk switching is ever made a task, and maybe it'd be significantly faster without those capture lambdas? Just wondering.

SafwatHalaby · 2016-02-07T20:59:52Z

My guess is that the current roll-your-own entity destructor queue is faster than the cWorld task system. I don't have any real numbers though.

tigerw · 2016-02-07T21:03:18Z

Dunno. Maybe, maybe not, maybe the compiler does magic to it, @worktycho knows. Everything else is using QueueTask though - imagine what it would be like if they all added their own queue structures to the world! Entities shouldn't be an exception.

SafwatHalaby · 2016-02-07T22:03:24Z

Done conversion to cWorld's tasks. Rebased and squashed. I like this.

SafwatHalaby · 2016-02-07T22:03:58Z

src/Chunk.cpp

@@ -139,6 +139,7 @@ cChunk::~cChunk()
 	{
 		if (!(*itr)->IsPlayer())
 		{
+			(*itr)->SetParentChunk(nullptr);  // To prevent the entity from queueing a future chunk removal.


change comment

SafwatHalaby · 2016-02-07T22:15:43Z

Doubletick prevention seems to be working :)

SafwatHalaby · 2016-02-07T22:19:45Z

I got a "moving to same chunk" error. I guess I'll leave the doubletick thing to a seperate PR ^_^

SafwatHalaby · 2016-02-07T22:21:24Z

When you move an entity to a new chunk which hasn't yet ticked, that entity will tick twice. The solution is moving it post-tick, preventing such doubleticks. Hence the cChunk::tick task queue. But it seems to be messing something.

SafwatHalaby · 2016-02-07T22:23:35Z

Rebased and removed doubletick prevention attempt.

SafwatHalaby · 2016-02-08T08:33:00Z

src/Entities/Player.cpp

+		LOGD("Warping player (%s) from world (%s) to (%s). Source chunk: (%d, %d) ",
+			this->GetName().c_str(),
+			a_OldWorld.GetName().c_str(), a_World->GetName().c_str(),
+			ParentChunk->GetPosX(), ParentChunk->GetPosZ()


Is markDirty required for players?

Better call it, so that it's consistent with the other entities.

But I see if entity is not a player then call markDirty everywhere.

SafwatHalaby · 2016-02-08T14:28:11Z

I realized this commit name is misleading. It's not about unloaded chunks, it's about loaded, nonticking chunks. There's no trivial way to access entities in unloaded chunks. (You'd need to load and check all unloaded chunks from storage).

SafwatHalaby · 2016-02-09T10:44:46Z

@sphinxc0re done.

SafwatHalaby · 2016-02-09T14:23:26Z

Please do not merge yet, I want to be completely sure I got all pointers right in all cases first.

SafwatHalaby · 2016-02-12T16:06:48Z

src/Chunk.cpp

+			// Mark as dirty if it was a server-generated entity:
+			if (!(*itr)->IsPlayer())
+			{
+				MarkDirty();


I think this line will save everyone thousands of false dirties a day. It seems that previously, anywhere you walk into would be marked dirty because the if player condition was missing.

SafwatHalaby · 2016-02-12T17:41:03Z

I have replaced IsDestroyed with IsTicking. IsDestroyed returns true only when an entity is about to be deleted. !IsTicking() returns true if the entity is either about to teleport or about to be deleted.

SafwatHalaby · 2016-02-12T17:43:24Z

I put all the player destruction logic in cClientHandle::~cClientHandle:. Previously, it was dispersed into 3 functions for no apparent reason: cClientHandle::Destroy(), cClientHandle::ClosedSocket(), cClientHandle::~cClientHandle(). Was there a reason for that? Did I miss anything here?

SafwatHalaby · 2016-02-12T17:49:38Z

src/Entities/Entity.h

 	void Destroy(bool a_ShouldBroadcast = true);

+	/** Destroy the entity without scheduling memory freeing. This should only be used by cChunk for internal memory management. */


by cChunk and cClientHandler

SafwatHalaby · 2016-02-12T17:54:59Z

What is wrong with the CI?

sphinxc0re · 2016-02-12T18:00:23Z

Nothing. GCC is throwing so many errors, that it stops counting and aborts

SafwatHalaby · 2016-02-12T21:14:50Z

It is throwing errors in unchanged code.

SafwatHalaby · 2016-02-12T21:17:32Z

src/ClientHandle.cpp

 	if (m_Player != nullptr)
 	{
+		cWorld * World = m_Player->GetWorld();
+		ASSERT(World != nullptr);


Should there really be an assert here?

tigerw · 2016-02-13T01:14:25Z

great, IsTicking came back...

SafwatHalaby · 2016-02-13T09:31:25Z

@tigerw IsDestroyed must be replaced. We need something which returns true for either destruction or teleportation, not just destruction.

tigerw · 2016-02-13T12:35:49Z

Could you explain a bit more the problem? Why does anything need to distinguish between these states?

SafwatHalaby · 2016-02-13T15:02:28Z

@tigerw Okay.

Why does anything need to distinguish between these states?

Actually, we do NOT need to distinguish between teleportation and destruction, and this is why I made this change.

For instance, cChunk previously had this:

for each entity
  if not IsDestroyed() then tick

The problem here is that teleporting entities will get ticked. So I replaced it with

for each entity
  if IsTicking() then tick

IsTicking() plays the role of the previous !IsDestroyed(), but now it returns a false for teleporting entities too, not just dying ones. In other words, I eliminated distinguishing between the two states.

SafwatHalaby · 2016-02-13T15:08:33Z

src/Entities/Entity.h

+	/** Returns true if the entity is valid and ticking. Returns false if the entity is not ticking.
+	When the entity is not ticking, it is either scheduled for teleportation or for destruction.
+	Either way, using it is dangerous. */
+	bool IsTicking(void) const;


I removed IsDestroyed. How do I deal with the LUA API here?

Perhaps I should export IsTicking, and make IsDestroyed a deprecated alias for !IsTicking()?.

...don't bother...

(don't tell anyone I said that)

SafwatHalaby · 2016-02-16T07:51:30Z

Todo: Document the new API functionIsTicking, mark IsDestroyed as deprecated, and that's it, I guess.

SafwatHalaby · 2016-02-16T14:55:24Z

Done.

tigerw · 2016-02-18T17:29:37Z

Looks good.

I shall, of course, take every available opportunity to tease you about ParentChunk :D

SafwatHalaby · 2016-02-19T05:10:14Z

@tigerw is evil!

[Queue approach] Fix entity destruction in unticking chunks

tigerw reviewed Feb 7, 2016
View reviewed changes

SafwatHalaby mentioned this pull request Feb 7, 2016

[Different approach] Fix entity destruction in unloaded chunks #2983

Closed

SafwatHalaby reviewed Feb 7, 2016
View reviewed changes

SafwatHalaby reviewed Feb 8, 2016
View reviewed changes

SafwatHalaby mentioned this pull request Feb 8, 2016

Remove world travel stability warning cuberite/users-manual#66

Closed

SafwatHalaby added this to the Stable world travel milestone Feb 11, 2016

SafwatHalaby reviewed Feb 12, 2016
View reviewed changes

SafwatHalaby reviewed Feb 13, 2016
View reviewed changes

Proper entity destruction in non-ticking chunks

83459d0

SafwatHalaby pushed a commit that referenced this pull request Feb 19, 2016

Merge pull request #2986 from LogicParrot/entityDestroy_approach4

a776337

[Queue approach] Fix entity destruction in unticking chunks

SafwatHalaby merged commit a776337 into cuberite:master Feb 19, 2016

SafwatHalaby deleted the entityDestroy_approach4 branch March 23, 2016 08:45

lkolbly mentioned this pull request Sep 5, 2017

Stabilise MoveToWorld #4004

Merged

		void Destroy(bool a_ShouldBroadcast = true);

		/** Destroy the entity without scheduling memory freeing. This should only be used by cChunk for internal memory management. */

[Queue approach] Fix entity destruction in unticking chunks #2986

[Queue approach] Fix entity destruction in unticking chunks #2986

Conversation

SafwatHalaby commented Feb 7, 2016

SafwatHalaby commented Feb 7, 2016

SafwatHalaby commented Feb 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigerw commented Feb 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SafwatHalaby commented Feb 7, 2016

SafwatHalaby commented Feb 7, 2016

SafwatHalaby commented Feb 7, 2016

SafwatHalaby commented Feb 7, 2016

tigerw commented Feb 7, 2016

tigerw commented Feb 7, 2016

SafwatHalaby commented Feb 7, 2016

tigerw commented Feb 7, 2016

SafwatHalaby commented Feb 7, 2016

tigerw commented Feb 7, 2016

SafwatHalaby commented Feb 7, 2016

SafwatHalaby commented Feb 7, 2016

tigerw commented Feb 7, 2016

SafwatHalaby commented Feb 7, 2016

Choose a reason for hiding this comment

SafwatHalaby commented Feb 7, 2016

SafwatHalaby commented Feb 7, 2016

SafwatHalaby commented Feb 7, 2016

SafwatHalaby commented Feb 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SafwatHalaby commented Feb 8, 2016

SafwatHalaby commented Feb 9, 2016

SafwatHalaby commented Feb 9, 2016

Choose a reason for hiding this comment

SafwatHalaby commented Feb 12, 2016

SafwatHalaby commented Feb 12, 2016

Choose a reason for hiding this comment

SafwatHalaby commented Feb 12, 2016

sphinxc0re commented Feb 12, 2016

SafwatHalaby commented Feb 12, 2016

Choose a reason for hiding this comment

tigerw commented Feb 13, 2016

SafwatHalaby commented Feb 13, 2016

tigerw commented Feb 13, 2016

SafwatHalaby commented Feb 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SafwatHalaby commented Feb 16, 2016

SafwatHalaby commented Feb 16, 2016

tigerw commented Feb 18, 2016

SafwatHalaby commented Feb 19, 2016