Large uptick in client crashes after moving to model pool/cache solution

In our game Aftermath, we previously did not use any kind of model pooling or caching when dealing with the gun models characters hold for their third person character. Upon switching to a cached solution, we are seeing a large increase in client crash rate. It does not look like there is any kind of leak in processes, nor in memory consumption. In fact, we do not really have a whole lot to go off of. What we do have, however, is a bunch of client crash .dmp files provided to us by our community members who seem to be crashing frequently. They are attached in the post privately.

Expected behavior

We hope you all at Roblox can take a look at these client crash reports and either tell us what we are doing incorrectly to cause these crashes, or hopefully fix the crashes on your end.

This is the cache module we have created for our project:

--!strict

--- This module serves as a cache service for PVInstances.
-- It stores PVInstance objects far from the origin to reduce the impact of reparenting highly volitile PVInstances.

export type PVInstanceCacheRegistry = {
	ReferencePVInstance: PVInstance, -- The reference PVInstance to clone from.
	ReleasedCache: { PVInstance },
	DeferredDelete: { PVInstance },
	Allocated: { PVInstance },
	PreferredCacheSize: number, -- Default is 40.
	CacheSizeMode: CacheSizeMode,
	TotalGenerated: number,
	TotalDeleted: number,
	Incrementer: number,
	StorageCFrame: CFrame,
};

type CacheSizeMode = 'DeferredDelete' | 'ReallocateOldest'

local PVInstanceCache = {};
PVInstanceCache.Registries = {} :: { [string]: PVInstanceCacheRegistry };
PVInstanceCache.DeferredDeleteRate = 0.25; -- Poll rate for the deferred delete loop.

local STORAGE_CFRAME = CFrame.new(0, 10e8, 0);

--- Creates a new PVInstance cache registry if one by the registry_name does not exist.
-- @param registry_name string the name associated with the PVInstanceCacheRegistry.
-- @param pv_instance PVInstance the PVInstance to use as a template for items in the cache.
-- @param preferred_cache_size number Optional the maximum preferred number of items in the PVInstanceCacheRegistry.
-- @returns PVInstanceCacheRegistry the created or existing PVInstanceCacheRegistry.
function PVInstanceCache:CreateRegistry(registry_name: string, pv_instance: PVInstance, preferred_cache_size: number?, storage_cframe: CFrame?): PVInstanceCacheRegistry
	assert(pv_instance:IsA('PVInstance'), '[PVInstanceCache.CreateRegistry] pv_instance must inherit from PVInstance.');

	if (not PVInstanceCache.Registries[registry_name]) then
		PVInstanceCache.Registries[registry_name] = {
			ReferencePVInstance = pv_instance;
			ReleasedCache = {},
			DeferredDelete = {},
			Allocated = {},
			CacheSizeMode = 'DeferredDelete',
			PreferredCacheSize = preferred_cache_size or 40,
			TotalGenerated = 0,
			TotalDeleted = 0,
			Incrementer = 0,
			StorageCFrame = storage_cframe or STORAGE_CFRAME,
		} :: PVInstanceCacheRegistry;
	end

	return PVInstanceCache.Registries[registry_name];
end

--- Checks if a registry exists with the given registry name.
function PVInstanceCache:HasRegistry(registry_name: string): boolean
	return not not PVInstanceCache.Registries[registry_name];
end

--- Sets the maximum preferred number of PVInstances in the specified registry.
function PVInstanceCache:SetRegistryCacheSize(registry_name: string, preferred_cache_size: number)
	local cache = PVInstanceCache.Registries[registry_name];
	if (cache) then
		cache.PreferredCacheSize = preferred_cache_size;
	end
end

--- Sets the maximum preferred number of PVInstances in the specified registry.
function PVInstanceCache:SetRegistryCacheSizeMode(registry_name: string, cache_size_mode: CacheSizeMode)
	local cache = PVInstanceCache.Registries[registry_name];
	if (cache) then
		cache.CacheSizeMode = cache_size_mode;
	end
end

function PVInstanceCache:Prewarm(registry_name: string, count: number)
	local instances_to_release: {PVInstance} = table.create(count)
	
	for i=1,count do
		local part, _ = PVInstanceCache:AllocatePVInstance(registry_name)
		if ( part ) then
			table.insert(instances_to_release, part)
		end
	end
	
	for _,v in instances_to_release do
		PVInstanceCache:ReleasePVInstance(registry_name, v)
	end
end


--- Allocates an instance in the PVInstance cache from the PVInstanceCacheRegistry.
-- When allocating a PVInstance, it is important to release it when you no longer need it
-- via ReleasePVInstance.
-- @returns PVInstance the allocated PVInstance
-- @returns boolean whether a new PVInstance was newly created or not.
function PVInstanceCache:AllocatePVInstance(registry_name: string): (PVInstance?, boolean, number)
	local cache = PVInstanceCache.Registries[registry_name];
	if (cache) then
		cache.Incrementer += 1;

		local allocation_id = cache.Incrementer;

		if (#cache.ReleasedCache > 0) then
			-- There's an available PVInstance in the released cache.
			local instance = table.remove(cache.ReleasedCache, 1) :: PVInstance;

			if (cache.CacheSizeMode == 'ReallocateOldest') then
				instance:SetAttribute('AllocationId', allocation_id);
			end

			table.insert(cache.Allocated, instance);

			return instance, false, allocation_id;
		elseif (#cache.DeferredDelete > 0) then
			local instance = table.remove(cache.DeferredDelete, 1) :: PVInstance;

			if (cache.CacheSizeMode == 'ReallocateOldest') then
				instance:SetAttribute('AllocationId', allocation_id);
			end

			table.insert(cache.Allocated, instance);

			-- There's an extra PVInstance available in the deferred delete list.
			return instance, false, allocation_id;
		else
			-- TODO: Perhaps perform some kind of analysis here to incorporate an autoscaling concept for the registry.
			cache.TotalGenerated += 1;
			local pv_instance = nil

			-- In the case that we're past our maximum number of items in the cache, and that we want to reallocate the oldest
			-- we go ahead and grab the oldest entry in the allocated cache.
			if (cache.CacheSizeMode == 'ReallocateOldest' and cache.TotalGenerated - cache.TotalDeleted > cache.PreferredCacheSize) then
				pv_instance = table.remove(cache.Allocated, 1);
			else
				-- Create a new PVInstance for the cache.
				local pv_i = cache.ReferencePVInstance:Clone() :: PVInstance;

				pv_i.Destroying:Connect(function()
					warn('PVInstance cache item is being destroyed!', pv_i:GetFullName(), pv_i:GetPivot())
				end)

				pv_i.AncestryChanged:Connect(function(_, parent)
					if (not parent) then
						--warn('PVInstance cache item was deleted!', registry_name, pv_i.Parent and pv_i.Parent:GetFullName())
						local allocated_index = table.find(cache.Allocated, pv_i);
						if (allocated_index) then
							table.remove(cache.Allocated, allocated_index);
						end

						local released_index = table.find(cache.ReleasedCache, pv_i);
						if (released_index) then
							table.remove(cache.ReleasedCache, released_index);
						end

						local deferred_index = table.find(cache.DeferredDelete, pv_i);
						if (deferred_index) then
							table.remove(cache.DeferredDelete, deferred_index);
						end
					end
				end);

				pv_instance = pv_i;
			end

			local instance = pv_instance :: PVInstance

			if (cache.CacheSizeMode == 'ReallocateOldest') then
				instance:SetAttribute('AllocationId', allocation_id);
			end

			table.insert(cache.Allocated, instance);

			return instance, true, allocation_id;
		end
	end

	return nil, false, -1;
end

--- Releases the specified PVInstance back to the PVInstanceCacheRegistry.
-- Checks if the cache size is larger than the preferred maximum cache size
-- if not, the item is reused later. If so, the item is set up to be destroyed in a deferred manner.
-- Items in the deferred delete list can be reused if there is nothing available in the cache and they
-- haven't been destroyed yet.
function PVInstanceCache:ReleasePVInstance(registry_name: string, pv_instance: PVInstance, keep_position: boolean?)
	local cache = PVInstanceCache.Registries[registry_name];
	if (cache) then

		local allocated_index = table.find(cache.Allocated, pv_instance);
		if (allocated_index) then
			table.remove(cache.Allocated, allocated_index);
		end

		if (#cache.ReleasedCache > cache.PreferredCacheSize) then
			-- Too many cached PVInstances, add this to the deferred delete list.
			table.insert(cache.DeferredDelete, pv_instance);
		else
			-- There's room in the cache, add to the released cache list.
			table.insert(cache.ReleasedCache, pv_instance);
		end

		-- Store the PVInstance outside of render distance.
		if (not keep_position) then
			pv_instance:PivotTo(cache.StorageCFrame);
		end
	end
end

task.spawn(function()
	while (true) do
		task.wait(PVInstanceCache.DeferredDeleteRate);

		for _, registry in pairs(PVInstanceCache.Registries) do
			if (#registry.DeferredDelete > 0 and registry.CacheSizeMode == 'DeferredDelete') then
				local instance = table.remove(registry.DeferredDelete, 1) :: PVInstance;
				instance:Destroy();
				registry.TotalDeleted += 1;
				task.wait();
			end
		end
	end
end);

return PVInstanceCache;

A private message is associated with this bug report

1 Like

Hi,
Thanks for the report.
The crash is happening inside the CPU render thread while preparing the scene to send to the GPU. I’ve forwarded the bug to the Rendering team.

3 Likes

We have had a look, but we don’t really understand how the code can end up in the state it is in. We’ve put out what we think is something that might prevent crashing, but it looks like either the model pool/cache solution was turned off Nov 13-14 or the problem went away.

We will continue looking in the mean time.

We disabled the weapon rendering in our game last week to try to mitigate the crashes. It definitely had an effect:


This is only for characters in workspace though. Weapon rendering in viewport frames, which use the pool solution, is still enabled. If you press “tab” while playing our game to view your inventory, the player there has its weapon attached.

We can re enable to test your latest changes.

Sorry for the long delay, we were in the middle of a big update and our remote config to reenable gun models wasn’t working :grimacing:

We were able to test this, and I can confirm that the issue is still happening. We use this “caching” concept all over the place. One, perhaps, unique thing in this case is that the gun models are welded. At the moment I weld these models to the character model in the position that we want, and when the weapon is no longer in use I disable the weld and move the model to the “cache” location.

I have a hunch that it has something to do with these welds, since it doesn’t seem to actually matter how far away from origin we move these. It was crashing at 0, 10000, 0 as well, which seems totally reasonable distance wise.

Unfortunately, the welds are a pretty important optimization step for us in this particular case. We don’t have the CPU headroom to CFrame them every step, so we would ideally prefer that the physics engine can just do its thing here.

We have a slightly similar concept with sounds, but we typically keep those just sitting around since they’re non-collide and invisible parts that the sounds live in.

Edit: We disabled the weapon models again since the crashing came back.

Thank you for checking and letting us know the issue is still there. That’s unfortunate and we will have to dig deeper so that a release in the new year can hopefully resolve it. Apologies that it is a problem you currently have to work around.

You have already sent us a private message with a test place, but if you do find additional information, please share that in a test place, as that will greatly improve the debugging & fixing time.

Not sure why this was tagged as fixed, it is not.

@TigerRabbit2 I tested by parenting the cached instances in nil instead, and the crashing issue did not happen. However, this is significantly slower, and causes some rather frustrating complexities in state management around the cache (i.e., parenting priority becomes messy).

Not sure if you all have looked into this further on your end, but it’s definitely still a fairly significant issue for us. It seems to me, that it is likely related to the welds.

Yes we continue to look into this. It is just different people than me at this point.

1 Like