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

tid dropped if >max(uint32) #886

Open
d4l3k opened this issue Sep 13, 2024 · 3 comments
Open

tid dropped if >max(uint32) #886

d4l3k opened this issue Sep 13, 2024 · 3 comments

Comments

@d4l3k
Copy link

d4l3k commented Sep 13, 2024

The thread IDs from JSON traces are getting dropped if the tid doesn't fit in a uint32. These are displayed finewith chrome://tracing but doesn't work with Perfetto.

I have py-spy chrometrace formatted traces in the format of:

{"args":{"filename":"\u003cfrozen importlib._bootstrap\u003e","line":688},"cat":"py-spy","name":"_load_unlocked","ph":"B","pid":10402,"tid":139627874611776,"ts":9720432}

These are I believe the pthreads thread ID and doesn't fit in a uint32.

opt_tid = json::CoerceToUint32(value["tid"]);

@LalitMaganti
Copy link
Collaborator

I'd suggest this is WAI from the Perfetto perspective: please see https://perfetto.dev/docs/faq#why-does-perfetto-not-support-lt-some-obscure-json-format-feature-gt-

@d4l3k
Copy link
Author

d4l3k commented Sep 13, 2024

@LalitMaganti looks like TrackEvent protos are also using int32 for thread ids so doesn't really solve the underlying problem regardless

As a hack it, does look fairly straightforward to just cast it as a string and set it as a threadname instead

std::optional<uint32_t> opt_tid;
if (value.isMember("tid")) {
if (value["tid"].isString()) {
// See the comment for |pid| string handling above: the same applies here.
StringId thread_name_id = storage->InternString(value["tid"].asCString());
opt_tid = thread_name_id.raw_id();
procs->UpdateThreadName(*opt_tid, thread_name_id,
ThreadNamePriority::kOther);
} else {
opt_tid = json::CoerceToUint32(value["tid"]);
}
}

@LalitMaganti
Copy link
Collaborator

looks like TrackEvent protos are also using int32 for thread ids so doesn't really solve the underlying problem regardless

Right: the main reason is that supporting int64 thread IDs can be quite a performance concern as it doubles the memory use of all places where tids are stored and when people use us for multi-GB traces, this adds up.

My response was more dealing with "why does this work on chrome tracing but not in Perfetto". You're correct that if you need int64 tid support and there's not a way to emit 32 bit tids instead then yes you'll need some hack.

As a hack it, does look fairly straightforward to just cast it as a string and set it as a threadname instead

That is quite a hack :) can we maybe instead directly cast from int64 -> int32 instead and store the tids in the casted form? We can expose the tids as the name as you suggest then.

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

No branches or pull requests

2 participants