Skip to content

RFC: Add blob feature - #938

Open
eibach wants to merge 7 commits into
wolfpld:masterfrom
eibach:feature_blob
Open

RFC: Add blob feature#938
eibach wants to merge 7 commits into
wolfpld:masterfrom
eibach:feature_blob

Conversation

@eibach

@eibach eibach commented Dec 3, 2024

Copy link
Copy Markdown
Contributor

This adds the ability to trace the status of arbitrary blobs in memory.

This is an RFC only, to see if I am thinking into the right direction. The encoding parameter is meant to add some decoding ability in the profiler later on. Documentation and timeline integration is still missing completely.

This adds the ability to trace the status of arbitrary blobs in memory.
@wolfpld

wolfpld commented Dec 3, 2024

Copy link
Copy Markdown
Owner

What is the intended use case?

Have you tried sending "large" blobs, i.e. larger than enum { TargetFrameSize = 256 * 1024 } bytes?

@eibach

eibach commented Dec 3, 2024

Copy link
Copy Markdown
Contributor Author

My use case is recording structs or packets on the fly so I can inspect them later. Intended size is << 64k, with 64k as upper limit.

@wolfpld

wolfpld commented Dec 3, 2024

Copy link
Copy Markdown
Owner

I'm not sure about imposing a limit. You may want instead to send the 3D scene geometry for debug purposes, and that could be easily in the megabytes range. (I actually did such a thing: https://bitbucket.org/wolfpld/tracy/commits/branch/mesh-debug)

@eibach

eibach commented Dec 3, 2024

Copy link
Copy Markdown
Contributor Author

Ok, I see. I have used the string infrastructure because I thought that identical blobs should only get stored and transferred once. Do you think that would also make sense for bigger blobs like the geometry from your usecase?

@wolfpld

wolfpld commented Dec 3, 2024

Copy link
Copy Markdown
Owner

Sorry, I don't remember the exact details of how things are transferred, so I can't comment on the specifics.

Hashing things for storage shouldn't be a problem, you can get tens of GB/s of hashing perf e.g. with xxh3.

Hashing things for transfer looks like a no-go for me, as you'd need to do it on the client, and doing extra calculations when you are profiling and want to keep impact minimal is something you'd generally want to avoid.

@eibach

eibach commented Dec 3, 2024

Copy link
Copy Markdown
Contributor Author

Ah yes, you are right, hashing only is done on the server side. I got that mixed up. I will check if I hit any limits when I crank up blob size.

@eibach

eibach commented Dec 3, 2024

Copy link
Copy Markdown
Contributor Author

Works only up to 64k - 1 bytes using the string infrastructure. So maybe I have to come up with a different transfer strategy...

@wolfpld

wolfpld commented Dec 3, 2024 via email

Copy link
Copy Markdown
Owner

@eibach

eibach commented Dec 3, 2024

Copy link
Copy Markdown
Contributor Author

Alright, I will see if I can get some inspiration from Worker::QueryDataTransfer()

@eibach

eibach commented Dec 9, 2024

Copy link
Copy Markdown
Contributor Author

I am struggling with the implementation of Worker::QueryDataTransfer(). Why is everything divided in pieces up to 12 bytes and not larger chunks? Doesn't that generate a lot of overhead? Probably I am missing the point here.

@wolfpld

wolfpld commented Dec 9, 2024

Copy link
Copy Markdown
Owner

Originally the query was 1 byte type + 8 bytes pointer, and that was enough. Then some queries needed more space, so an additional 4 bytes were added, which are unused in all other queries. Splitting larger data into 12-byte chunks is a result of retrofitting this system for, well, larger data.

Yes, it's very inefficient, but the only thing being transferred this way are source filename paths that the client may have available, so it's not a big deal.

While the implementation can be improved, there are special precautions that need to be taken for things to work properly. The client-to-server data pipe is basically not limited in any way, as it requires high bandwidth. The kernel will send a certain number of packets over the wire, and then wait for a confirmation of retrieval before continuing. Now, if the receiving end also wants to send a large amount of data, it will never read what was received, and these confirmations will never be delivered, resulting in a deadlock. The fixed size of server query packets is an implementation detail that makes it easier to calculate how much data can be sent before a read must occur.

Note that this is only a consideration for server-to-client communication. The memory blobs you want to send are from client to server, and the much larger available size limit (256 KB) is another unrelated implementation detail.

@eibach

eibach commented Dec 9, 2024

Copy link
Copy Markdown
Contributor Author

This is my idea for the client side. If you agree, I would start implementing the server side.

Comment thread public/client/TracyProfiler.cpp Outdated
memcpy( ptr, data, size );

TracyQueuePrepare( callstack == 0 ? QueueType::Blob : QueueType::BlobCallstack );
MemWrite( &item->blobData.time, GetTime() );

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Time value needs to be send as a delta over the wire in Dequeue().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation is taken from the Message...() functions. As far as my understanding goes, it is converted in TracyWorker using the TscTime() function.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The missing processing step is done in Deqeue(). Look for refCtx for examples of this.

Comment thread public/common/TracyQueue.hpp Outdated
Comment thread public/common/TracyQueue.hpp Outdated

struct QueueBlobData : public QueueBlob
{
uint64_t encoding;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the meaning of the encoding values? The TracyBlob() macro requires setting a value. How will user know what to set there? If "the encoding parameter is meant to add some decoding ability in the profiler later on", how will this future functionality be able to work, when there is no "restricted for future use" set of values that shouldn't be used by users?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not completely done with this. My (totally untested) idea was to have a facility for executing user supplied dissectors, like in Wireshark, maybe even Lua based. This facility could use the encoding value to choose the dissector.

@eibach

eibach commented Dec 16, 2024

Copy link
Copy Markdown
Contributor Author

I have added support for fragmented transfer of blob data. Now uint32_t should be the limit ;)

@wolfpld

wolfpld commented Dec 17, 2024

Copy link
Copy Markdown
Owner

Why is m_serverQuerySpaceLeft increased? The client should send all the blob fragments in succession, and the server doesn't make any queries?

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 this pull request may close these issues.

3 participants