Skip to content

Issue 6989 - Create a better string representation of Tid's for easier thread identification and debugging. #1582

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

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 17, 2013

http://d.puremagic.com/issues/show_bug.cgi?id=6989

I'm thinking of actually implementing an opEquals method as well, which simply would compare the threadID fields.

Edit: Nevermind about opEquals, it's unnecessary because this is automatic thanks to the .tupleof == .tupleof rewrite on == aggregate expressions.

Maybe there's an even better way to implement toString, perhaps by inspecting mbox or some other field. But I figured this would be the simplest way to do it without depending on OS-specific thread identifiers.

… easier thread identification and debugging.
void toString(scope void delegate(const(char)[]) sink) const
{
sink("Tid(");
sink(format("%s", threadID)); // avoid std.conv import
Copy link
Author

Choose a reason for hiding this comment

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

Btw, I've used the sink version of toString since I want to avoid memory allocation, but then again I'm not sure if we have a way to avoid the allocations when converting a size_t to a string. Can format itself take a sink? If so I could just redirect it, e.g. (pseudocode):

formatSink(sink, "%s", threadID);

Or whatever the function name is if it exists.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, std.format.formattedWrite is supposed to work with a sink, but I don't think it will work at present. I believe @monarchdodra has been spending time on this problem recently.

I guess a possible work-around would be to use sformat with a buffer big enough for any possible value of threadID; that shouldn't be hard. I hear that sformat can still allocate heap memory even on success though, but surely it's an improvement over straight-up format, especially with the future in mind.

edit:

BTW, as long as a formatting function is used, it's probably best to put the Tid(...) enclosing right there in the format string too, to reduce the calls to the sink.

Copy link
Collaborator

Choose a reason for hiding this comment

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

formatValue (and formattedWrite), should work with a sink. The only limitation is that none of the passed args arguments are strings, or wide characters:
http://d.puremagic.com/issues/show_bug.cgi?id=10571

For wide chars, it is an obvious and stright up bug.

For strings, it's more complex. I'd need to double check, but there is a code branch for raw write ("%r") of strings that makes formattedWrite iterate over the string as range of dchars, and you can't palce a dchar in a string sink. Since I have no idea what "raw" is actually supposed to do, I don't even know if its correct behavior or not to do it that way. My fix works around that problem, but I think a deeper review of what formatValue actually does would be needed.

If you do need to put a string into the sink, then chances are you don't actually need to format it, so a straight up "put" will workaround that problem.

Anywhoo, back to the subject. TL;DR; In your case,

formattedWrite(sink, "Tid(%s)", threadID);

Should 100% work no problem, and not allocate either (as far as the write is concerned of course, the sink itself can do anything it wants).

Copy link
Member

Choose a reason for hiding this comment

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

It should not allocate, but apparently it still does, and that's not talking about the obvious case of allocating exceptions in case of failure. Anyway, not really relevant to this PR...

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should not allocate, but apparently it still does

Really? How do you know? And where does it do so?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since I have no idea what "raw" is actually supposed to do

I investigated, and I came to the conclusion that it is completly wrong. I'll have to fix this directly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I investigated, and I came to the conclusion that it is completly wrong. I'll have to fix this directly.

I fixed it! :)

It was just silly meta mistakes.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, ideally this would be formattedWrite(sink, "Tid(%s)", threadID); but using sink(threadID) is much better than format.

@ghost ghost closed this Sep 17, 2013
@ghost
Copy link
Author

ghost commented Sep 17, 2013

Closing until I figure out the appropriate fix.

@ghost
Copy link
Author

ghost commented Sep 17, 2013

Also worth noting that storing the ID In the struct would increase the memory size of Tid, which may not be what we want.

Anyway, before I go forward again I need to figure out how to get the thread identifier of the thread that owns the Tid, probably through the message box somehow.

@ghost
Copy link

ghost commented Sep 2, 2014

Naive question, but why not just store Thread.getThis.toHash in struct Tid? Will Thread.toHash() change during the lifetime of Tid?

@ghost
Copy link
Author

ghost commented Sep 2, 2014

@klamonte: You could give this a try if you want to make a pull request. :)

@ghost
Copy link

ghost commented Sep 2, 2014

Naive question was naive. But let's give it a shot anyway. ;-)

#2482

This pull request was closed.
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.

4 participants