dotnet-gcdump: Update docs to include print verb

[mustakimali]

Proposing new verb for reading from a gcdump and write the result in stdout.

The PR for the code is here: #791

[josalem]

Thanks @mustakimali!

I've talked a little bit offline with Noah about this. One thing we chatted about is that there is the possibility we could extract different types of data from these gcdumps besides heap stats like this. Theoretically, we could display root information or some other grouping of the data. It might make sense to use a verb like report in the vein of perf report .... It would make sense in the context of the tool: a user invokes dotnet gcdump collect ... and follows it up with dotnet gcdump report ... similar to perf. We could then add morereport types as time goes on, even adding things like multi-dump diffing.

In this model, I'm not sure having the --std-out option would make sense. Perhaps it could be replaced with --report and it would invoke the reporting logic after the collection, much like the --format flag on dotnet trace invokes the convert logic after collection.

CC @noahfalk & @shirhatti

[noahfalk]

Thanks for contributing to the tools! I like where this is going and I made a few suggestions on the design.

[josalem]

I think Noah's points are good. In my mind the following make sense:

dotnet gcdump collect ... --report <report-type># generates a file and then immediately prints the specified report

and

dotnet gcdump report <report-type> -f <filename># prints the specified report for an existing file

and

dotnet gcdump report <report-type> -p <pid># collects a gcdump from the specified process and prints the report WITHOUT writing a .gcdump file

[sywhang]

dotnet gcdump collect ... --report <report-type> # generates a file and then immediately prints the specified report

dotnet gcdump report <report-type> -p <pid> # collects a gcdump from the specified process and prints the report WITHOUT writing a .gcdump file

These two commands seem way too similar and might be confusing when the user tries to do one or the other. Should we rather just go with one over the other instead of having both?

[josalem]

I just read through the perf man files looking for prior art on this, and I think Sung's point is valid. We should choose one or the other but not both to avoid semantic dissonance between the verb-flag combos.

[josalem]

I'd like to move this PR forward so we can get around to merging its predecessor. If there isn't any more discussion on the naming, I propose we go with the following:

dotnet gcdump report [-t|--report-type] <report-type> [[-p|--pid <pid>] | [--addresss|--diagnostics-server-address <address>] | [-f|--files <file[,file[...]]>]]

i.e., we introduce the report verb, and have flags for

• consuming a file(s) (post-mortem analysis with option to eventually support multiple files for diffing)
• consuming a pid or diag server address to connect, take a gcdump, and report to stdout without saving a file

N.B. - The --address|--diagnostics-server-address flag is being added in #770

[mustakimali]

@josalem apologies for not getting back to this sooner, I will make sure I make some time for this, this weekend.

What's that <report-type> is for? is it to determine whether to connect to a diagnostic server or reading from a file? in that case the flag --address and -p would be mutually exclusive?

I also noted @noahfalk 's suggestion on formatting the output: #809 (comment)

[josalem]

I intended <report-type> to be the type of the report being generated. Your other PR introduces a heapstat report, but I would imagine we would want to add future report types like diffs, roots, etc. The --addresss and -p flags are mutually exclusive. I may have butchered the CLI command optional/required bracing in my comment 😅.

[mustakimali]

Thanks got it 😄

[mustakimali]

Thanks for the great feedback. I've pushed the code and also updated the docs with the changes we discussed above. Let me know what do you think.

Also please look at a comment above: #809 (comment)

Code is in this separate PR: #791

[noahfalk]

Its not critical, but including the count of objects would be nice if it is possible. I unresolved the earlier comment about it and added the API that I believe should do the job MemoryGraph.GetHistogramByType()

[mustakimali]

I briefly looked for something like that. I'll add that into the report. 👍

[mustakimali]

I am bit confused here, is that TypeIdx property corresponds to the index of memoryGraph.m_types array?

[mustakimali]

Does this look like a correct approach? just eying the result looks like it's not giving me the correct count of objects.
https://github.com/dotnet/diagnostics/pull/791/files#diff-7fc6cf484dfedf7c72a124738460b1dfR105-R109

[noahfalk]

Thanks @mustakimali for your great work on this! I commented on a few small things that hopefully can be adjusted, but even if they can't I am happy to sign off. I'll leave it to @josalem to merge whenever you guys are ready.

[josalem]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments 👍

[josalem]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments 👍

[mustakimali]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments +1

I have just one question
#809 (comment)

we can discuss this in the implementation PR. Then this PR is ready to be merged. 🙏

[mustakimali]

Proposing new verb for reading from a gcdump and write the result in stdout.

The PR for the code is here: #791

[josalem]

Thanks @mustakimali!

I've talked a little bit offline with Noah about this. One thing we chatted about is that there is the possibility we could extract different types of data from these gcdumps besides heap stats like this. Theoretically, we could display root information or some other grouping of the data. It might make sense to use a verb like report in the vein of perf report .... It would make sense in the context of the tool: a user invokes dotnet gcdump collect ... and follows it up with dotnet gcdump report ... similar to perf. We could then add morereport types as time goes on, even adding things like multi-dump diffing.

In this model, I'm not sure having the --std-out option would make sense. Perhaps it could be replaced with --report and it would invoke the reporting logic after the collection, much like the --format flag on dotnet trace invokes the convert logic after collection.

CC @noahfalk & @shirhatti

[noahfalk]

Thanks for contributing to the tools! I like where this is going and I made a few suggestions on the design.

[josalem]

I think Noah's points are good. In my mind the following make sense:

dotnet gcdump collect ... --report <report-type># generates a file and then immediately prints the specified report

and

dotnet gcdump report <report-type> -f <filename># prints the specified report for an existing file

and

dotnet gcdump report <report-type> -p <pid># collects a gcdump from the specified process and prints the report WITHOUT writing a .gcdump file

[sywhang]

dotnet gcdump collect ... --report <report-type> # generates a file and then immediately prints the specified report

dotnet gcdump report <report-type> -p <pid> # collects a gcdump from the specified process and prints the report WITHOUT writing a .gcdump file

These two commands seem way too similar and might be confusing when the user tries to do one or the other. Should we rather just go with one over the other instead of having both?

[josalem]

I just read through the perf man files looking for prior art on this, and I think Sung's point is valid. We should choose one or the other but not both to avoid semantic dissonance between the verb-flag combos.

[josalem]

I'd like to move this PR forward so we can get around to merging its predecessor. If there isn't any more discussion on the naming, I propose we go with the following:

dotnet gcdump report [-t|--report-type] <report-type> [[-p|--pid <pid>] | [--addresss|--diagnostics-server-address <address>] | [-f|--files <file[,file[...]]>]]

i.e., we introduce the report verb, and have flags for

• consuming a file(s) (post-mortem analysis with option to eventually support multiple files for diffing)
• consuming a pid or diag server address to connect, take a gcdump, and report to stdout without saving a file

N.B. - The --address|--diagnostics-server-address flag is being added in #770

[mustakimali]

@josalem apologies for not getting back to this sooner, I will make sure I make some time for this, this weekend.

What's that <report-type> is for? is it to determine whether to connect to a diagnostic server or reading from a file? in that case the flag --address and -p would be mutually exclusive?

I also noted @noahfalk 's suggestion on formatting the output: #809 (comment)

[josalem]

I intended <report-type> to be the type of the report being generated. Your other PR introduces a heapstat report, but I would imagine we would want to add future report types like diffs, roots, etc. The --addresss and -p flags are mutually exclusive. I may have butchered the CLI command optional/required bracing in my comment 😅.

[mustakimali]

Thanks got it 😄

[mustakimali]

Thanks for the great feedback. I've pushed the code and also updated the docs with the changes we discussed above. Let me know what do you think.

Also please look at a comment above: #809 (comment)

Code is in this separate PR: #791

[noahfalk]

Its not critical, but including the count of objects would be nice if it is possible. I unresolved the earlier comment about it and added the API that I believe should do the job MemoryGraph.GetHistogramByType()

[mustakimali]

I briefly looked for something like that. I'll add that into the report. 👍

[mustakimali]

I am bit confused here, is that TypeIdx property corresponds to the index of memoryGraph.m_types array?

[mustakimali]

Does this look like a correct approach? just eying the result looks like it's not giving me the correct count of objects.
https://github.com/dotnet/diagnostics/pull/791/files#diff-7fc6cf484dfedf7c72a124738460b1dfR105-R109

[noahfalk]

Thanks @mustakimali for your great work on this! I commented on a few small things that hopefully can be adjusted, but even if they can't I am happy to sign off. I'll leave it to @josalem to merge whenever you guys are ready.

[josalem]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments 👍

[josalem]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments 👍

[mustakimali]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments +1

I have just one question
#809 (comment)

we can discuss this in the implementation PR. Then this PR is ready to be merged. 🙏

[mustakimali]

Proposing new verb for reading from a gcdump and write the result in stdout.

The PR for the code is here: #791

[josalem]

Thanks @mustakimali!

I've talked a little bit offline with Noah about this. One thing we chatted about is that there is the possibility we could extract different types of data from these gcdumps besides heap stats like this. Theoretically, we could display root information or some other grouping of the data. It might make sense to use a verb like report in the vein of perf report .... It would make sense in the context of the tool: a user invokes dotnet gcdump collect ... and follows it up with dotnet gcdump report ... similar to perf. We could then add morereport types as time goes on, even adding things like multi-dump diffing.

In this model, I'm not sure having the --std-out option would make sense. Perhaps it could be replaced with --report and it would invoke the reporting logic after the collection, much like the --format flag on dotnet trace invokes the convert logic after collection.

CC @noahfalk & @shirhatti

[noahfalk]

Thanks for contributing to the tools! I like where this is going and I made a few suggestions on the design.

[josalem]

I think Noah's points are good. In my mind the following make sense:

dotnet gcdump collect ... --report <report-type># generates a file and then immediately prints the specified report

and

dotnet gcdump report <report-type> -f <filename># prints the specified report for an existing file

and

dotnet gcdump report <report-type> -p <pid># collects a gcdump from the specified process and prints the report WITHOUT writing a .gcdump file

[sywhang]

dotnet gcdump collect ... --report <report-type> # generates a file and then immediately prints the specified report

dotnet gcdump report <report-type> -p <pid> # collects a gcdump from the specified process and prints the report WITHOUT writing a .gcdump file

These two commands seem way too similar and might be confusing when the user tries to do one or the other. Should we rather just go with one over the other instead of having both?

[josalem]

I just read through the perf man files looking for prior art on this, and I think Sung's point is valid. We should choose one or the other but not both to avoid semantic dissonance between the verb-flag combos.

[josalem]

I'd like to move this PR forward so we can get around to merging its predecessor. If there isn't any more discussion on the naming, I propose we go with the following:

dotnet gcdump report [-t|--report-type] <report-type> [[-p|--pid <pid>] | [--addresss|--diagnostics-server-address <address>] | [-f|--files <file[,file[...]]>]]

i.e., we introduce the report verb, and have flags for

• consuming a file(s) (post-mortem analysis with option to eventually support multiple files for diffing)
• consuming a pid or diag server address to connect, take a gcdump, and report to stdout without saving a file

N.B. - The --address|--diagnostics-server-address flag is being added in #770

[mustakimali]

@josalem apologies for not getting back to this sooner, I will make sure I make some time for this, this weekend.

What's that <report-type> is for? is it to determine whether to connect to a diagnostic server or reading from a file? in that case the flag --address and -p would be mutually exclusive?

I also noted @noahfalk 's suggestion on formatting the output: #809 (comment)

[josalem]

I intended <report-type> to be the type of the report being generated. Your other PR introduces a heapstat report, but I would imagine we would want to add future report types like diffs, roots, etc. The --addresss and -p flags are mutually exclusive. I may have butchered the CLI command optional/required bracing in my comment 😅.

[mustakimali]

Thanks got it 😄

[mustakimali]

Thanks for the great feedback. I've pushed the code and also updated the docs with the changes we discussed above. Let me know what do you think.

Also please look at a comment above: #809 (comment)

Code is in this separate PR: #791

[noahfalk]

Its not critical, but including the count of objects would be nice if it is possible. I unresolved the earlier comment about it and added the API that I believe should do the job MemoryGraph.GetHistogramByType()

[mustakimali]

I briefly looked for something like that. I'll add that into the report. 👍

[mustakimali]

I am bit confused here, is that TypeIdx property corresponds to the index of memoryGraph.m_types array?

[mustakimali]

Does this look like a correct approach? just eying the result looks like it's not giving me the correct count of objects.
https://github.com/dotnet/diagnostics/pull/791/files#diff-7fc6cf484dfedf7c72a124738460b1dfR105-R109

[noahfalk]

Thanks @mustakimali for your great work on this! I commented on a few small things that hopefully can be adjusted, but even if they can't I am happy to sign off. I'll leave it to @josalem to merge whenever you guys are ready.

[josalem]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments 👍

[josalem]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments 👍

[mustakimali]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments +1

I have just one question
#809 (comment)

we can discuss this in the implementation PR. Then this PR is ready to be merged. 🙏

[mustakimali]

Proposing new verb for reading from a gcdump and write the result in stdout.

The PR for the code is here: #791

[josalem]

Thanks @mustakimali!

I've talked a little bit offline with Noah about this. One thing we chatted about is that there is the possibility we could extract different types of data from these gcdumps besides heap stats like this. Theoretically, we could display root information or some other grouping of the data. It might make sense to use a verb like report in the vein of perf report .... It would make sense in the context of the tool: a user invokes dotnet gcdump collect ... and follows it up with dotnet gcdump report ... similar to perf. We could then add morereport types as time goes on, even adding things like multi-dump diffing.

In this model, I'm not sure having the --std-out option would make sense. Perhaps it could be replaced with --report and it would invoke the reporting logic after the collection, much like the --format flag on dotnet trace invokes the convert logic after collection.

CC @noahfalk & @shirhatti

[noahfalk]

Thanks for contributing to the tools! I like where this is going and I made a few suggestions on the design.

[josalem]

I think Noah's points are good. In my mind the following make sense:

dotnet gcdump collect ... --report <report-type># generates a file and then immediately prints the specified report

and

dotnet gcdump report <report-type> -f <filename># prints the specified report for an existing file

and

dotnet gcdump report <report-type> -p <pid># collects a gcdump from the specified process and prints the report WITHOUT writing a .gcdump file

[sywhang]

dotnet gcdump collect ... --report <report-type> # generates a file and then immediately prints the specified report

dotnet gcdump report <report-type> -p <pid> # collects a gcdump from the specified process and prints the report WITHOUT writing a .gcdump file

These two commands seem way too similar and might be confusing when the user tries to do one or the other. Should we rather just go with one over the other instead of having both?

[josalem]

I just read through the perf man files looking for prior art on this, and I think Sung's point is valid. We should choose one or the other but not both to avoid semantic dissonance between the verb-flag combos.

[josalem]

I'd like to move this PR forward so we can get around to merging its predecessor. If there isn't any more discussion on the naming, I propose we go with the following:

dotnet gcdump report [-t|--report-type] <report-type> [[-p|--pid <pid>] | [--addresss|--diagnostics-server-address <address>] | [-f|--files <file[,file[...]]>]]

i.e., we introduce the report verb, and have flags for

• consuming a file(s) (post-mortem analysis with option to eventually support multiple files for diffing)
• consuming a pid or diag server address to connect, take a gcdump, and report to stdout without saving a file

N.B. - The --address|--diagnostics-server-address flag is being added in #770

[mustakimali]

@josalem apologies for not getting back to this sooner, I will make sure I make some time for this, this weekend.

What's that <report-type> is for? is it to determine whether to connect to a diagnostic server or reading from a file? in that case the flag --address and -p would be mutually exclusive?

I also noted @noahfalk 's suggestion on formatting the output: #809 (comment)

[josalem]

I intended <report-type> to be the type of the report being generated. Your other PR introduces a heapstat report, but I would imagine we would want to add future report types like diffs, roots, etc. The --addresss and -p flags are mutually exclusive. I may have butchered the CLI command optional/required bracing in my comment 😅.

[mustakimali]

Thanks got it 😄

[mustakimali]

Thanks for the great feedback. I've pushed the code and also updated the docs with the changes we discussed above. Let me know what do you think.

Also please look at a comment above: #809 (comment)

Code is in this separate PR: #791

[noahfalk]

Its not critical, but including the count of objects would be nice if it is possible. I unresolved the earlier comment about it and added the API that I believe should do the job MemoryGraph.GetHistogramByType()

[mustakimali]

I briefly looked for something like that. I'll add that into the report. 👍

[mustakimali]

I am bit confused here, is that TypeIdx property corresponds to the index of memoryGraph.m_types array?

[mustakimali]

Does this look like a correct approach? just eying the result looks like it's not giving me the correct count of objects.
https://github.com/dotnet/diagnostics/pull/791/files#diff-7fc6cf484dfedf7c72a124738460b1dfR105-R109

[noahfalk]

Thanks @mustakimali for your great work on this! I commented on a few small things that hopefully can be adjusted, but even if they can't I am happy to sign off. I'll leave it to @josalem to merge whenever you guys are ready.

[josalem]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments 👍

[josalem]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments 👍

[mustakimali]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments +1

I have just one question
#809 (comment)

we can discuss this in the implementation PR. Then this PR is ready to be merged. 🙏

[mustakimali]

Proposing new verb for reading from a gcdump and write the result in stdout.

The PR for the code is here: #791

[josalem]

Thanks @mustakimali!

I've talked a little bit offline with Noah about this. One thing we chatted about is that there is the possibility we could extract different types of data from these gcdumps besides heap stats like this. Theoretically, we could display root information or some other grouping of the data. It might make sense to use a verb like report in the vein of perf report .... It would make sense in the context of the tool: a user invokes dotnet gcdump collect ... and follows it up with dotnet gcdump report ... similar to perf. We could then add morereport types as time goes on, even adding things like multi-dump diffing.

In this model, I'm not sure having the --std-out option would make sense. Perhaps it could be replaced with --report and it would invoke the reporting logic after the collection, much like the --format flag on dotnet trace invokes the convert logic after collection.

CC @noahfalk & @shirhatti

[noahfalk]

Thanks for contributing to the tools! I like where this is going and I made a few suggestions on the design.

[josalem]

I think Noah's points are good. In my mind the following make sense:

dotnet gcdump collect ... --report <report-type># generates a file and then immediately prints the specified report

and

dotnet gcdump report <report-type> -f <filename># prints the specified report for an existing file

and

dotnet gcdump report <report-type> -p <pid># collects a gcdump from the specified process and prints the report WITHOUT writing a .gcdump file

[sywhang]

dotnet gcdump collect ... --report <report-type> # generates a file and then immediately prints the specified report

dotnet gcdump report <report-type> -p <pid> # collects a gcdump from the specified process and prints the report WITHOUT writing a .gcdump file

These two commands seem way too similar and might be confusing when the user tries to do one or the other. Should we rather just go with one over the other instead of having both?

[josalem]

I just read through the perf man files looking for prior art on this, and I think Sung's point is valid. We should choose one or the other but not both to avoid semantic dissonance between the verb-flag combos.

[josalem]

I'd like to move this PR forward so we can get around to merging its predecessor. If there isn't any more discussion on the naming, I propose we go with the following:

dotnet gcdump report [-t|--report-type] <report-type> [[-p|--pid <pid>] | [--addresss|--diagnostics-server-address <address>] | [-f|--files <file[,file[...]]>]]

i.e., we introduce the report verb, and have flags for

• consuming a file(s) (post-mortem analysis with option to eventually support multiple files for diffing)
• consuming a pid or diag server address to connect, take a gcdump, and report to stdout without saving a file

N.B. - The --address|--diagnostics-server-address flag is being added in #770

[mustakimali]

@josalem apologies for not getting back to this sooner, I will make sure I make some time for this, this weekend.

What's that <report-type> is for? is it to determine whether to connect to a diagnostic server or reading from a file? in that case the flag --address and -p would be mutually exclusive?

I also noted @noahfalk 's suggestion on formatting the output: #809 (comment)

[josalem]

I intended <report-type> to be the type of the report being generated. Your other PR introduces a heapstat report, but I would imagine we would want to add future report types like diffs, roots, etc. The --addresss and -p flags are mutually exclusive. I may have butchered the CLI command optional/required bracing in my comment 😅.

[mustakimali]

Thanks got it 😄

[mustakimali]

Thanks for the great feedback. I've pushed the code and also updated the docs with the changes we discussed above. Let me know what do you think.

Also please look at a comment above: #809 (comment)

Code is in this separate PR: #791

[noahfalk]

Its not critical, but including the count of objects would be nice if it is possible. I unresolved the earlier comment about it and added the API that I believe should do the job MemoryGraph.GetHistogramByType()

[mustakimali]

I briefly looked for something like that. I'll add that into the report. 👍

[mustakimali]

I am bit confused here, is that TypeIdx property corresponds to the index of memoryGraph.m_types array?

[mustakimali]

Does this look like a correct approach? just eying the result looks like it's not giving me the correct count of objects.
https://github.com/dotnet/diagnostics/pull/791/files#diff-7fc6cf484dfedf7c72a124738460b1dfR105-R109

[noahfalk]

Thanks @mustakimali for your great work on this! I commented on a few small things that hopefully can be adjusted, but even if they can't I am happy to sign off. I'll leave it to @josalem to merge whenever you guys are ready.

[josalem]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments 👍

[josalem]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments 👍

[mustakimali]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments +1

I have just one question
#809 (comment)

we can discuss this in the implementation PR. Then this PR is ready to be merged. 🙏

[mustakimali]

Proposing new verb for reading from a gcdump and write the result in stdout.

The PR for the code is here: #791

[josalem]

Thanks @mustakimali!

I've talked a little bit offline with Noah about this. One thing we chatted about is that there is the possibility we could extract different types of data from these gcdumps besides heap stats like this. Theoretically, we could display root information or some other grouping of the data. It might make sense to use a verb like report in the vein of perf report .... It would make sense in the context of the tool: a user invokes dotnet gcdump collect ... and follows it up with dotnet gcdump report ... similar to perf. We could then add morereport types as time goes on, even adding things like multi-dump diffing.

In this model, I'm not sure having the --std-out option would make sense. Perhaps it could be replaced with --report and it would invoke the reporting logic after the collection, much like the --format flag on dotnet trace invokes the convert logic after collection.

CC @noahfalk & @shirhatti

[noahfalk]

Thanks for contributing to the tools! I like where this is going and I made a few suggestions on the design.

[josalem]

I think Noah's points are good. In my mind the following make sense:

dotnet gcdump collect ... --report <report-type># generates a file and then immediately prints the specified report

and

dotnet gcdump report <report-type> -f <filename># prints the specified report for an existing file

and

dotnet gcdump report <report-type> -p <pid># collects a gcdump from the specified process and prints the report WITHOUT writing a .gcdump file

[sywhang]

dotnet gcdump collect ... --report <report-type> # generates a file and then immediately prints the specified report

dotnet gcdump report <report-type> -p <pid> # collects a gcdump from the specified process and prints the report WITHOUT writing a .gcdump file

These two commands seem way too similar and might be confusing when the user tries to do one or the other. Should we rather just go with one over the other instead of having both?

[josalem]

I just read through the perf man files looking for prior art on this, and I think Sung's point is valid. We should choose one or the other but not both to avoid semantic dissonance between the verb-flag combos.

[josalem]

I'd like to move this PR forward so we can get around to merging its predecessor. If there isn't any more discussion on the naming, I propose we go with the following:

dotnet gcdump report [-t|--report-type] <report-type> [[-p|--pid <pid>] | [--addresss|--diagnostics-server-address <address>] | [-f|--files <file[,file[...]]>]]

i.e., we introduce the report verb, and have flags for

• consuming a file(s) (post-mortem analysis with option to eventually support multiple files for diffing)
• consuming a pid or diag server address to connect, take a gcdump, and report to stdout without saving a file

N.B. - The --address|--diagnostics-server-address flag is being added in #770

[mustakimali]

@josalem apologies for not getting back to this sooner, I will make sure I make some time for this, this weekend.

What's that <report-type> is for? is it to determine whether to connect to a diagnostic server or reading from a file? in that case the flag --address and -p would be mutually exclusive?

I also noted @noahfalk 's suggestion on formatting the output: #809 (comment)

[josalem]

I intended <report-type> to be the type of the report being generated. Your other PR introduces a heapstat report, but I would imagine we would want to add future report types like diffs, roots, etc. The --addresss and -p flags are mutually exclusive. I may have butchered the CLI command optional/required bracing in my comment 😅.

[mustakimali]

Thanks got it 😄

[mustakimali]

Thanks for the great feedback. I've pushed the code and also updated the docs with the changes we discussed above. Let me know what do you think.

Also please look at a comment above: #809 (comment)

Code is in this separate PR: #791

[noahfalk]

Its not critical, but including the count of objects would be nice if it is possible. I unresolved the earlier comment about it and added the API that I believe should do the job MemoryGraph.GetHistogramByType()

[mustakimali]

I briefly looked for something like that. I'll add that into the report. 👍

[mustakimali]

I am bit confused here, is that TypeIdx property corresponds to the index of memoryGraph.m_types array?

[mustakimali]

Does this look like a correct approach? just eying the result looks like it's not giving me the correct count of objects.
https://github.com/dotnet/diagnostics/pull/791/files#diff-7fc6cf484dfedf7c72a124738460b1dfR105-R109

[noahfalk]

Thanks @mustakimali for your great work on this! I commented on a few small things that hopefully can be adjusted, but even if they can't I am happy to sign off. I'll leave it to @josalem to merge whenever you guys are ready.

[josalem]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments 👍

[josalem]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments 👍

[mustakimali]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments +1

I have just one question
#809 (comment)

we can discuss this in the implementation PR. Then this PR is ready to be merged. 🙏

[mustakimali]

Proposing new verb for reading from a gcdump and write the result in stdout.

The PR for the code is here: #791

[josalem]

Thanks @mustakimali!

I've talked a little bit offline with Noah about this. One thing we chatted about is that there is the possibility we could extract different types of data from these gcdumps besides heap stats like this. Theoretically, we could display root information or some other grouping of the data. It might make sense to use a verb like report in the vein of perf report .... It would make sense in the context of the tool: a user invokes dotnet gcdump collect ... and follows it up with dotnet gcdump report ... similar to perf. We could then add morereport types as time goes on, even adding things like multi-dump diffing.

In this model, I'm not sure having the --std-out option would make sense. Perhaps it could be replaced with --report and it would invoke the reporting logic after the collection, much like the --format flag on dotnet trace invokes the convert logic after collection.

CC @noahfalk & @shirhatti

[noahfalk]

Thanks for contributing to the tools! I like where this is going and I made a few suggestions on the design.

[josalem]

I think Noah's points are good. In my mind the following make sense:

dotnet gcdump collect ... --report <report-type># generates a file and then immediately prints the specified report

and

dotnet gcdump report <report-type> -f <filename># prints the specified report for an existing file

and

dotnet gcdump report <report-type> -p <pid># collects a gcdump from the specified process and prints the report WITHOUT writing a .gcdump file

[sywhang]

dotnet gcdump collect ... --report <report-type> # generates a file and then immediately prints the specified report

dotnet gcdump report <report-type> -p <pid> # collects a gcdump from the specified process and prints the report WITHOUT writing a .gcdump file

These two commands seem way too similar and might be confusing when the user tries to do one or the other. Should we rather just go with one over the other instead of having both?

[josalem]

I just read through the perf man files looking for prior art on this, and I think Sung's point is valid. We should choose one or the other but not both to avoid semantic dissonance between the verb-flag combos.

[josalem]

I'd like to move this PR forward so we can get around to merging its predecessor. If there isn't any more discussion on the naming, I propose we go with the following:

dotnet gcdump report [-t|--report-type] <report-type> [[-p|--pid <pid>] | [--addresss|--diagnostics-server-address <address>] | [-f|--files <file[,file[...]]>]]

i.e., we introduce the report verb, and have flags for

• consuming a file(s) (post-mortem analysis with option to eventually support multiple files for diffing)
• consuming a pid or diag server address to connect, take a gcdump, and report to stdout without saving a file

N.B. - The --address|--diagnostics-server-address flag is being added in #770

[mustakimali]

@josalem apologies for not getting back to this sooner, I will make sure I make some time for this, this weekend.

What's that <report-type> is for? is it to determine whether to connect to a diagnostic server or reading from a file? in that case the flag --address and -p would be mutually exclusive?

I also noted @noahfalk 's suggestion on formatting the output: #809 (comment)

[josalem]

I intended <report-type> to be the type of the report being generated. Your other PR introduces a heapstat report, but I would imagine we would want to add future report types like diffs, roots, etc. The --addresss and -p flags are mutually exclusive. I may have butchered the CLI command optional/required bracing in my comment 😅.

[mustakimali]

Thanks got it 😄

[mustakimali]

Thanks for the great feedback. I've pushed the code and also updated the docs with the changes we discussed above. Let me know what do you think.

Also please look at a comment above: #809 (comment)

Code is in this separate PR: #791

[noahfalk]

Its not critical, but including the count of objects would be nice if it is possible. I unresolved the earlier comment about it and added the API that I believe should do the job MemoryGraph.GetHistogramByType()

[mustakimali]

I briefly looked for something like that. I'll add that into the report. 👍

[mustakimali]

I am bit confused here, is that TypeIdx property corresponds to the index of memoryGraph.m_types array?

[mustakimali]

Does this look like a correct approach? just eying the result looks like it's not giving me the correct count of objects.
https://github.com/dotnet/diagnostics/pull/791/files#diff-7fc6cf484dfedf7c72a124738460b1dfR105-R109

[noahfalk]

Thanks @mustakimali for your great work on this! I commented on a few small things that hopefully can be adjusted, but even if they can't I am happy to sign off. I'll leave it to @josalem to merge whenever you guys are ready.

[josalem]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments 👍

[josalem]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments 👍

[mustakimali]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments +1

I have just one question
#809 (comment)

we can discuss this in the implementation PR. Then this PR is ready to be merged. 🙏

[mustakimali]

Proposing new verb for reading from a gcdump and write the result in stdout.

The PR for the code is here: #791

[josalem]

Thanks @mustakimali!

I've talked a little bit offline with Noah about this. One thing we chatted about is that there is the possibility we could extract different types of data from these gcdumps besides heap stats like this. Theoretically, we could display root information or some other grouping of the data. It might make sense to use a verb like report in the vein of perf report .... It would make sense in the context of the tool: a user invokes dotnet gcdump collect ... and follows it up with dotnet gcdump report ... similar to perf. We could then add morereport types as time goes on, even adding things like multi-dump diffing.

In this model, I'm not sure having the --std-out option would make sense. Perhaps it could be replaced with --report and it would invoke the reporting logic after the collection, much like the --format flag on dotnet trace invokes the convert logic after collection.

CC @noahfalk & @shirhatti

[noahfalk]

Thanks for contributing to the tools! I like where this is going and I made a few suggestions on the design.

[josalem]

I think Noah's points are good. In my mind the following make sense:

dotnet gcdump collect ... --report <report-type># generates a file and then immediately prints the specified report

and

dotnet gcdump report <report-type> -f <filename># prints the specified report for an existing file

and

dotnet gcdump report <report-type> -p <pid># collects a gcdump from the specified process and prints the report WITHOUT writing a .gcdump file

[sywhang]

dotnet gcdump collect ... --report <report-type> # generates a file and then immediately prints the specified report

dotnet gcdump report <report-type> -p <pid> # collects a gcdump from the specified process and prints the report WITHOUT writing a .gcdump file

These two commands seem way too similar and might be confusing when the user tries to do one or the other. Should we rather just go with one over the other instead of having both?

[josalem]

I just read through the perf man files looking for prior art on this, and I think Sung's point is valid. We should choose one or the other but not both to avoid semantic dissonance between the verb-flag combos.

[josalem]

I'd like to move this PR forward so we can get around to merging its predecessor. If there isn't any more discussion on the naming, I propose we go with the following:

dotnet gcdump report [-t|--report-type] <report-type> [[-p|--pid <pid>] | [--addresss|--diagnostics-server-address <address>] | [-f|--files <file[,file[...]]>]]

i.e., we introduce the report verb, and have flags for

• consuming a file(s) (post-mortem analysis with option to eventually support multiple files for diffing)
• consuming a pid or diag server address to connect, take a gcdump, and report to stdout without saving a file

N.B. - The --address|--diagnostics-server-address flag is being added in #770

[mustakimali]

@josalem apologies for not getting back to this sooner, I will make sure I make some time for this, this weekend.

What's that <report-type> is for? is it to determine whether to connect to a diagnostic server or reading from a file? in that case the flag --address and -p would be mutually exclusive?

I also noted @noahfalk 's suggestion on formatting the output: #809 (comment)

[josalem]

I intended <report-type> to be the type of the report being generated. Your other PR introduces a heapstat report, but I would imagine we would want to add future report types like diffs, roots, etc. The --addresss and -p flags are mutually exclusive. I may have butchered the CLI command optional/required bracing in my comment 😅.

[mustakimali]

Thanks got it 😄

[mustakimali]

Thanks for the great feedback. I've pushed the code and also updated the docs with the changes we discussed above. Let me know what do you think.

Also please look at a comment above: #809 (comment)

Code is in this separate PR: #791

[noahfalk]

Its not critical, but including the count of objects would be nice if it is possible. I unresolved the earlier comment about it and added the API that I believe should do the job MemoryGraph.GetHistogramByType()

[mustakimali]

I briefly looked for something like that. I'll add that into the report. 👍

[mustakimali]

I am bit confused here, is that TypeIdx property corresponds to the index of memoryGraph.m_types array?

[mustakimali]

Does this look like a correct approach? just eying the result looks like it's not giving me the correct count of objects.
https://github.com/dotnet/diagnostics/pull/791/files#diff-7fc6cf484dfedf7c72a124738460b1dfR105-R109

[noahfalk]

Thanks @mustakimali for your great work on this! I commented on a few small things that hopefully can be adjusted, but even if they can't I am happy to sign off. I'll leave it to @josalem to merge whenever you guys are ready.

[josalem]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments 👍

[josalem]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments 👍

[mustakimali]

I'd like to echo Noah on this: thanks for all the good work! I think this is just about ready for merge and we con focus on the implementation PR. I'll merge this after you see to Noah's comments +1

I have just one question
#809 (comment)

we can discuss this in the implementation PR. Then this PR is ready to be merged. 🙏