Various Garbage Generation Optimizations #53

oniietzschan · 2019-09-28T19:46:38Z

Hey kikito,

Here's a great big PR with various changes I've made in service of reducing the amount of garbage generated by Bump. Some of these changes I would expect to slightly increase the CPU time used by certain operations. However, in my experience garbage collection tends to be big bottleneck for keeping frametimes low, so in my view this is still a very worthwhile tradeoff. Hopefully you'll agree.

I'm going to be going through and leaving comments explaining the changes I've made. So if you happen to catch this right as I send it, you'll probably want to wait for me to finish writing those comments.

I've included the benchmark which I used while making these changes. It inserts a bunch of objects and world:move()s them around for a couple hundred generations. It's random, but is set to used a fixed seed, so the operations it performs should be identical across tests. By my testing, my changes cause bump generate less than 1/3rd of the garbage it used to. Here are the results I get on my machine:

anzu@Toki:~/git/bump$ luajit -v
LuaJIT 2.1.0-beta3 -- Copyright (C) 2005-2017 Mike Pall. http://luajit.org/

anzu@Toki:~/git/bump$ luajit performance_test.lua
============= Original =============
Garbage: 1741.58 kB
(Average after 10 tests.)
============= Modded =============
Garbage: 529.91 kB
(Average after 10 tests.)
anzu@Toki:~/git/bump$

anzu@Toki:~/git/bump$ lua -v
Lua 5.1.5  Copyright (C) 1994-2012 Lua.org, PUC-Rio

anzu@Toki:~/git/bump$ lua performance_test.lua
============= Original =============
Garbage: 3086.53 kB
(Average after 10 tests.)
============= Modded =============
Garbage: 996.72 kB
(Average after 10 tests.)

For what it's worth, these changes are live in my game Vacant Kingdom on Steam, which has been played (to some extent...) by several thousand people at this point. No bugs related to this have come to my attention, so I'd consider these changes to be fairly safe. The unit tests are passing too, of course.

Please have a look through and let me know which changes you're okay with, and I'll send you an updated PR with just those changes, okay?

Thanks, shru

* Use table pool for visited sets. * Use table pool for item dictionary. * Removed itemRect and otherRect from collision data. * Added distance to collision data. * Added World:projectMove() method.

…rn nil collisions instead of {}

coveralls · 2019-09-28T19:48:29Z

Coverage increased (+0.9%) to 97.344% when pulling 100641a on oniietzschan:2d into 7cae5d1 on kikito:master.

coveralls · 2019-09-28T19:48:29Z

Coverage increased (+0.8%) to 97.229% when pulling d01d827 on oniietzschan:2d into 7cae5d1 on kikito:master.

oniietzschan · 2019-09-28T19:53:38Z

bump.lua

@@ -28,6 +28,39 @@ local bump = {
  ]]
 }



So, the first change here is to add a table pool so that we can reuse temporary tables instead of throwing them away. This is used for a few operations:

The table used to keep track of visited items in World:project().

The table used to store the results of getDictItemsInCellRect

A new table used to keep track of visited items during World:projectMove() (formerly World:check()). More on this later.

oniietzschan · 2019-09-28T19:57:20Z

bump.lua

@@ -194,8 +227,7 @@ local function rect_detectCollision(x1,y1,w1,h1, x2,y2,w2,h2, goalX, goalY)
    move      = {x = dx, y = dy},
    normal    = {x = nx, y = ny},
    touch     = {x = tx, y = ty},


Stop returning itemRect and otherRect, because I found them to be simply unecessary in my own use. If a user needs them, they can simply call World:getRect(), if they don't need them, then we've saved some garbage right here.

We cache distance here, because it will often always be needed for sortByTiAndDistance. And when it is needed, it saves time to calculate it once here, rather than every time table.sort() needs to compare two objects.

EDIT: If you're okay with this being merged, I'll update README.md to reflect this change.

Although it occurs to me now that this is not "distance", but rather "squared distance". I wonder if it's worthwhile to math.sqrt() this... hmmm

oniietzschan · 2019-09-28T19:59:05Z

bump.lua

@@ -266,16 +298,16 @@ end
 -- Responses
 ------------------------------------------



Adding alreadyVisited is part of a change which I'll explain below by World:check() / World:projectMove()

oniietzschan · 2019-09-28T20:01:33Z

bump.lua

  assertIsRect(x,y,w,h)

  goalX = goalX or x
  goalY = goalY or y
-  filter  = filter  or defaultFilter
+  filter = filter or defaultFilter



We don't create the table for collisions until we've actually encountered a collision. This definitely cuts down on garbage collection, but also adds a branch to the logic. I believe it's worthwhile, but maybe you'll disagree.

oniietzschan · 2019-09-28T20:02:23Z

bump.lua

@@ -467,7 +501,7 @@ function World:project(item, x,y,w,h, goalX, goalY, filter)
  local dictItemsInCellRect = getDictItemsInCellRect(self, cl,ct,cw,ch)

  for other,_ in pairs(dictItemsInCellRect) do


Again, see World:check() / World:projectMove() for more details on how alreadyVisited works.

oniietzschan · 2019-09-28T20:06:00Z

bump.lua

+  if collisions ~= nil then
+    table.sort(collisions, sortByTiAndDistance)
+  end
+


EMPTY_TABLE is by far the newest change here, everything else I changed back in march. The idea is that we reuse one static {} whenever we return results with no collisions. Formerly, for my own use, I was just returning nil, 0. But I think this is a little nicer because there's no API change.

It's ever so slightly dangerous though, because if the user does something crazy and inserts into EMPTY_TABLE, that could cause some incredibly strange behavior.

oniietzschan · 2019-09-28T20:10:05Z

bump.lua

@@ -696,19 +742,23 @@ function World:move(item, goalX, goalY, filter)
 end



I added a new method World:projectMove(). This is essentially a combination of World:check() and World:project(). You can use it to check collisions for items which aren't actually stored inside the world. Just like :project(), you can use items which aren't actually inside the Bump world. Just like :check(), it simulates collision responses as the item "moves" totwards its goal.

I've found this incredibly practical for optimization, because the fewer things which are stored in the world, the faster collision detection is.

An example of how you could use this: Bullets need to collide with Actors; however, Actors don't necessarily need to collide with Bullets. So, we insert actors into the bump world, but we keep track of the bullets some other way. We can still have the bullets interact with the actors by using World:projectMove(bullet, ...).

Again, I'll document this in README if you're okay with this change.

oniietzschan · 2019-09-28T20:19:18Z

bump.lua

+  return self:projectMove(item, x, y, w, h, goalX,goalY, filter)
+end
+
+function World:projectMove(item, x, y, w, h, goalX, goalY, filter)
  filter = filter or defaultFilter



Okay, so here's the big alreadyVisited change I forecasted a couple times earlier. Really, the behavior here hasn't changed, but the implementation has.

Before: filter is wrapped with another function which keeps track of the "visited" items during collision response resolution. This creates a single use function every single time World:check() is invoked, a big source of garbage!

Now: Instead of wrapping filter, we just pass a visited table into the collision response functions, it still keeps track of "visited" items the same way, but without the waste.

oniietzschan · 2019-09-28T20:21:08Z

performance_test.lua

@@ -0,0 +1,74 @@
+local RNG_SEED = 69420731


Here's a the benchmark I was using during development. Not sure if it makes sense to merge it in. Let me know if you want this removed or moved to a subfolder or something.

oniietzschan · 2019-09-28T20:23:53Z

spec/World_spec.lua

@@ -407,5 +413,27 @@ describe('World', function()
        assert.same({0,-3,1,1}, {world:getRect(a)})
      end)
    end)
+


It's been a long time since I've thought about this, but I believe this was a test case I added to catch a bug I introduced (and fixed) while rejiggering visitedFilter / alreadyVisited.

oniietzschan added 10 commits March 27, 2019 21:54

Various memory optimizations.

27886eb

* Use table pool for visited sets. * Use table pool for item dictionary. * Removed itemRect and otherRect from collision data. * Added distance to collision data. * Added World:projectMove() method.

Fix tests.

899bc92

Save more memory: World:move(), :check(), and :projectMove() now retu…

8687554

…rn nil collisions instead of {}

Further decrease memory usage.

fecce4a

Final garbage generation tweak for now?

62767b6

fixup: luacheck

c06e0bc

Fixed bug and added unit test to catch it.

28fdc3f

fixup.

53b475f

Remove table pool unreturned tables debugging stuff.

45b50fe

Never return 'nil' collisions, return EMPTY_TABLE instead.

d01d827

oniietzschan commented Sep 28, 2019

View reviewed changes

Remove old debugging spec.

af767f4

oniietzschan commented Sep 28, 2019

View reviewed changes

oniietzschan added 2 commits September 29, 2019 15:44

Fix flakey test.

100641a

Use fewer tables for collision data.

392da11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various Garbage Generation Optimizations #53

Various Garbage Generation Optimizations #53

oniietzschan commented Sep 28, 2019

coveralls commented Sep 28, 2019 •

edited

Loading

coveralls commented Sep 28, 2019

oniietzschan Sep 28, 2019

oniietzschan Sep 28, 2019 •

edited

Loading

oniietzschan Sep 30, 2019

oniietzschan Sep 28, 2019 •

edited

Loading

oniietzschan Sep 28, 2019

oniietzschan Sep 28, 2019

oniietzschan Sep 28, 2019

oniietzschan Sep 28, 2019 •

edited

Loading

oniietzschan Sep 28, 2019

oniietzschan Sep 28, 2019

oniietzschan Sep 28, 2019

		@@ -266,16 +298,16 @@ end
		-- Responses
		------------------------------------------

		@@ -467,7 +501,7 @@ function World:project(item, x,y,w,h, goalX, goalY, filter)
		local dictItemsInCellRect = getDictItemsInCellRect(self, cl,ct,cw,ch)

		for other,_ in pairs(dictItemsInCellRect) do

		@@ -696,19 +742,23 @@ function World:move(item, goalX, goalY, filter)
		end

Various Garbage Generation Optimizations #53

Are you sure you want to change the base?

Various Garbage Generation Optimizations #53

Conversation

oniietzschan commented Sep 28, 2019

coveralls commented Sep 28, 2019 • edited Loading

coveralls commented Sep 28, 2019

Choose a reason for hiding this comment

oniietzschan Sep 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oniietzschan Sep 28, 2019 • edited Loading

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

oniietzschan Sep 28, 2019 • edited Loading

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

coveralls commented Sep 28, 2019 •

edited

Loading

oniietzschan Sep 28, 2019 •

edited

Loading

oniietzschan Sep 28, 2019 •

edited

Loading

oniietzschan Sep 28, 2019 •

edited

Loading