Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Slow HeightfieldShape Save/RestoreBinaryState #1080

Closed
attilaz opened this issue Apr 29, 2024 · 2 comments · Fixed by #1083
Closed

Slow HeightfieldShape Save/RestoreBinaryState #1080

attilaz opened this issue Apr 29, 2024 · 2 comments · Fixed by #1083

Comments

@attilaz
Copy link

attilaz commented Apr 29, 2024

We have a 8192x8192 heightfield. RestoreBinaryState takes 1231ms on a Galaxy A51 phone.

Most of the time spent with reading vector elements (mRangeBlocks, mHeightSamples, mActiveEdges)

Read(outT[i]);

I have modified the StreamIn vector reader to read the whole vector data with one call. This takes 114ms on the same phone.

I am not sure about is_trivial_v maybe it should be is_trivially_copyable?


template <class T, class A>
void				Read(std::vector<T, A>& outT)
{
	typename Array<T>::size_type len = outT.size(); // Initialize to previous array size, this is used for validation in the StateRecorder class
	Read(len);
	if (!IsEOF() && !IsFailed())
	{
		outT.resize(len);

		if (std::is_trivial_v<T>)
		{
			if (len > 0)
				ReadBytes(&outT[0], len * sizeof(outT[0]));
		}
		else
		{
			for (typename Array<T>::size_type i = 0; i < len; ++i)
				Read(outT[i]);
		}
		}
	else
		outT.clear();
	}

Our deserialization code looks like this. The ReadBytes is a virtual function which gets called with small sizes.

struct MyStreamInWrapper : public StreamIn
{
	MyStreamInWrapper(const uint8_t* start, const uint8_t* end) : _rover(start), _end(end)
	{
	}

	virtual void		ReadBytes(void* outData, size_t inNumBytes)
	{
		_eof = (_rover == _end);
		int copy = std::min(inNumBytes, (size_t)(_end - _rover));
		memcpy(outData, _rover, copy);
		_rover += copy;
	}

	/// Returns true when an attempt has been made to read past the end of the file
	virtual bool		IsEOF() const {
		return _eof;
			}

	/// Returns true if there was an IO failure
	virtual bool		IsFailed() const { return false; }

	const uint8_t* _rover;
	const uint8_t* _end;
	bool _eof = false;
		};

MyStreamInWrapper stream_in((uint8_t*)f->getData(), (uint8_t*)f->getData() + f->getDataSize());

// Load the shape
// If you have assigned custom ID's on save, you need to ensure that the shapes exist in this map on restore too.
JPH::Shape::ShapeResult result;

JPH::Shape::IDToShapeMap id_to_shape;
JPH::Shape::IDToMaterialMap id_to_material;
result = JPH::Shape::sRestoreWithChildren(stream_in, id_to_shape, id_to_material);

if (result.IsValid())
{
	restored_shape = result.Get();
}

@jrouwe
Copy link
Owner

jrouwe commented Apr 29, 2024

That's a good find!

Unfortunately, the suggested solution won't work as e.g. Vec3 is is_trivial and is_trivially_copyable but it requires calling the custom Read(Vec3 &) function to avoid writing garbage for the unused W component.

Let me cook something up.

jrouwe added a commit that referenced this issue Apr 30, 2024
Reading arrays of simple types in a single read instead of per element.

Fixes #1080
jrouwe added a commit that referenced this issue Apr 30, 2024
Reading arrays of simple types in a single read instead of per element.

Fixes #1080
@jrouwe
Copy link
Owner

jrouwe commented Apr 30, 2024

Should be fixed now, thanks for reporting this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants