Support for sample filtering by thread by aaalexandrov · Pull Request #86 · VerySleepy/verysleepy

aaalexandrov · 2021-08-18T20:18:24Z

Implement source thread-aware sample gathering, file format and display in the UI.
Add sample filtering by thread.
Add symbol per-thread statistics UI.
Fixes #70

CyberShadow · 2021-08-19T06:22:38Z

I can't open old .sleepy files with this change, could you please check?
We should support loading old files even when the format for new files changes.

aaalexandrov · 2021-08-19T10:43:49Z

I did not add compatibility code for opening the old file format files because I saw no compatibility code there for any old versions.
There didn't seem to be any infrastructure for that in place, aside from comparing to the current version and aborting in case it's different.
I can add loading from 0.90 specifically, but not backwards from there.
Out of curiosity, how often does this format change?

CyberShadow · 2021-08-19T10:53:37Z

I did not add compatibility code for opening the old file format files because I saw no compatibility code there for any old versions.

You're right, my mistake. I thought I remembered some compatibility code, but I guess I was wrong. So, I take that back (though of course, speaking of a separate enhancement, compatibility would be nice if possible).

Out of curiosity, how often does this format change?

Well, the 0.90 version was bumped in 2014...

aaalexandrov · 2021-08-19T11:03:14Z

I'll do compatibility from 0.90 in the coming days, it should be straightforward enough.

CyberShadow · 2021-08-19T11:07:59Z

OK, only if you want (my initial statement was based on a bad assumption, and practically speaking, it's easy enough to download an older version to open older files).

Since you're working with threads, maybe looking at #4 would be a better use of your time.

As for this PR, I will try to have a closer look at it and merge it soon.

aaalexandrov · 2021-08-19T12:19:22Z

I'll see what I can do about sampling newly spawned threads.
Is there a better way to detect them than just polling and comparing to the threads we're already tracking?

CyberShadow · 2021-08-19T12:23:17Z

I believe that a long time ago, @rmitton tried to implement this by making Very Sleepy also work as a debugger, attaching itself to the process. That would allow it to be notified of new threads using CREATE_PROCESS_DEBUG_EVENT.

There is code for this in src/profiler/debugger.cpp, but it was never finished.

See also #20.

CyberShadow

Here are my observations so far. I am going to try and see if this results in any changes in performance in practice.

src/appinfo.h

src/profiler/profilerthread.cpp

src/profiler/profiler.h

CyberShadow · 2021-08-21T06:03:35Z

src/profiler/symbolinfo.cpp

-			for (size_t n=0;n<modules.size();n++)
+			for (size_t m=0;m<modules.size();m++)
 			{
-				Module &mod = modules[n];
+				Module &mod = modules[m];


It might be too late for that, but generally it's better to place unrelated changes in separate commits. That would also provide for a place to describe why the change is done (i.e. which compiler warning is fixed if any), in the commit message.

I assumed you'd prefer a single commit. I squashed what few separate commits I had into one before pushing, sorry about that.
The local variable name changes here and there are to fix class / outer scope variables being shadowed.

Some good advice can be found by asking the right question:

https://www.google.com/search?q=when+to+commit+git

For instance:

You shouldn't commit based on a time basis, but on a feature basis. Whenever you add a new feature that's worth committing, commit. You added a working method? Commit. You fixed a typo? Commit. You fixed a file's wrong indentation? Commit. There's nothing wrong committing small changes, as soon as the commit is relevant.

What is wrong is committing a huge number of changes, with no relations between each others. It makes it very hard to detect the commit source of a given regression.

-- https://softwareengineering.stackexchange.com/a/74893/16481

CyberShadow · 2021-08-21T06:05:15Z

src/profiler/threadinfo.cpp

-// DE: 20090325 Threads now have CPU usage
-ThreadInfo::ThreadInfo(DWORD id_, HANDLE thread_handle_)
-:	id(id_), thread_handle(thread_handle_)
+std::wstring getThreadDescriptorName(HANDLE thread_handle)


Refactoring this function out also would ideally be done in its own commit.

src/wxProfilerGUI/CallstackView.cpp

src/wxProfilerGUI/database.cpp

src/wxProfilerGUI/CallstackView.cpp

src/wxProfilerGUI/database.cpp

CyberShadow · 2021-08-21T11:20:45Z

src/profiler/symbolinfo.cpp

-			for (size_t n=0;n<modules.size();n++)
+			for (size_t m=0;m<modules.size();m++)
 			{
-				Module &mod = modules[n];
+				Module &mod = modules[m];


Some good advice can be found by asking the right question:

https://www.google.com/search?q=when+to+commit+git

For instance:

You shouldn't commit based on a time basis, but on a feature basis. Whenever you add a new feature that's worth committing, commit. You added a working method? Commit. You fixed a typo? Commit. You fixed a file's wrong indentation? Commit. There's nothing wrong committing small changes, as soon as the commit is relevant.

What is wrong is committing a huge number of changes, with no relations between each others. It makes it very hard to detect the commit source of a given regression.

-- https://softwareengineering.stackexchange.com/a/74893/16481

src/wxProfilerGUI/database.cpp

CyberShadow · 2021-08-21T11:25:20Z

src/wxProfilerGUI/proclist.cpp

-struct ExclusivePred  { bool operator () (const Database::Item &a, const Database::Item &b) { return a.exclusive          < b.exclusive         ; } };
-struct InclusivePred  { bool operator () (const Database::Item &a, const Database::Item &b) { return a.inclusive          < b.inclusive         ; } };
+struct ExclusivePred  { bool operator () (const Database::Item &a, const Database::Item &b) {
+	if (a.exclusive != b.exclusive)
+		return a.exclusive < b.exclusive;
+	return a.inclusive < b.inclusive;
+} };
+struct InclusivePred  { bool operator () (const Database::Item &a, const Database::Item &b) {
+	if (a.inclusive != b.inclusive)
+		return a.inclusive < b.inclusive;
+	return a.exclusive < b.exclusive;
+} };


Nice, but ideally should be in its own commit. As it is there is zero documentation about this improvement.

src/wxProfilerGUI/threadsview.cpp

CyberShadow · 2021-08-21T14:29:10Z

Great! Thank you, and I don't have any more suggestions :)

Since I have a good setup for it, I am going to try and massage the git history a bit to ungroup unrelated changes from the big commits and squash changes that are not relevant to the history.

aaalexandrov · 2021-08-21T16:14:39Z

Thanks, if there's anything I missed, don't hesitate to say.
I'll probably do the issue about profiling newly spawned threads at some point, but it'll take me some time.

Use the other time datum as a secondary sort criteria.

- Record sample information for each thread - Record thread names - Remove IPCounts.txt, and instead reconstruct them at load time - Bump file format version - Add UI for viewing sample threads - Add UI for filtering by selected threads

Speed up worst case merging/insertion behavior.

…ckView

CyberShadow

Let me know if you agree with my history refactoring and commit messages, and we can merge it.

CyberShadow · 2021-08-21T17:30:25Z

src/wxProfilerGUI/profilergui.cpp

+		if (mostBusy)
 			threadHandles.push_back(mostBusy);
 		return threadHandles;
+	}


There might be a better way to silence this warning.

Depending on the compiler, sometimes wrapping the entire if expression in another set of parens does it.

aaalexandrov · 2021-08-21T18:01:48Z

It looks great to me, thanks for separating the commits.

CyberShadow · 2021-08-21T18:03:29Z

Thanks for the very nice contribution :)

CyberShadow reviewed Aug 21, 2021

View reviewed changes

aaalexandrov added 9 commits August 21, 2021 17:33

Fix identifier shadowing

d1d6daf

Improve process list sorting

9c0bf0e

Use the other time datum as a secondary sort criteria.

Avoid assignment in if expression

cd8a525

Add support for sample filtering by thread

df8b67a

- Record sample information for each thread - Record thread names - Remove IPCounts.txt, and instead reconstruct them at load time - Bump file format version - Add UI for viewing sample threads - Add UI for filtering by selected threads

Change database samples storage to map instead of sorted array

197e5e2

Speed up worst case merging/insertion behavior.

Optimize sample filtering after change of sample storage to use a map

3b93f91

Fix symbol averages stats for recursive functions

612e7bc

Use Schwartz transform to optimize thread sample filtering in Callsta…

d20063f

…ckView

Force refreshing callstacks when filter threads change

377f41e

CyberShadow approved these changes Aug 21, 2021

View reviewed changes

CyberShadow merged commit 08937c1 into VerySleepy:master Aug 21, 2021

Conversation

aaalexandrov commented Aug 18, 2021

Uh oh!

CyberShadow commented Aug 19, 2021

Uh oh!

aaalexandrov commented Aug 19, 2021

Uh oh!

CyberShadow commented Aug 19, 2021

Uh oh!

aaalexandrov commented Aug 19, 2021

Uh oh!

CyberShadow commented Aug 19, 2021

Uh oh!

aaalexandrov commented Aug 19, 2021

Uh oh!

CyberShadow commented Aug 19, 2021

Uh oh!

CyberShadow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CyberShadow Aug 21, 2021

Choose a reason for hiding this comment

Uh oh!

aaalexandrov Aug 21, 2021

Choose a reason for hiding this comment

Uh oh!

CyberShadow Aug 21, 2021

Choose a reason for hiding this comment

Uh oh!

CyberShadow Aug 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CyberShadow Aug 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyberShadow Aug 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyberShadow commented Aug 21, 2021

Uh oh!

aaalexandrov commented Aug 21, 2021

Uh oh!

CyberShadow left a comment

Choose a reason for hiding this comment

Uh oh!

CyberShadow Aug 21, 2021

Choose a reason for hiding this comment

Uh oh!

aaalexandrov commented Aug 21, 2021

Uh oh!

CyberShadow commented Aug 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants