Add a profiler and instrumentation #30

TheCurle · 2024-04-05T15:25:55Z

This PR adds three bits of infrastructure surrounding the usage of a Profiler:

Precise Timer

Currently, the Timer is a set of static ints, updated every frame by the ShadowApplication.
For the purposes of profiling, this is not useful (as we may want to profile many things that happen within a frame, with high precision).

SH::Timer is now a struct with a bunch of static methods, so that you can use it both as an RAII timer (create an instance at the start of a method to start the timer, and it stops when it goes out of scope. Call elapsed() to get the amount of nanoseconds that have elapsed since the timer started.

The timer struct itself uses std::chrono to perform timing, but the profiler may wish to get the exact current timestamp.
Thus,

static size_t getTimestamp();

is provided. To get maximum precision on every system, there is a platform source that implements this function on both Windows and Linux using platform-specific instrumentation calls.

Threads

The profiler needs to be able to handle multiple parallel threads - we'll have a job system. multithreaded rendering, a UI thread, stuff like that.
However, if we simply use std::thread for these, it's not possible to attach a profiler to this thread (as the thread handle is buried deep inside the stdlib), which is no good.
Thus, we have our own way to manage threads - the Thread class.
Simply implement Thread and provide a Run method, everything else is handled automatically.
A Join method is provided to block main thread waiting for the given thread, and the thread can also Wait() and Notify() to allow for complex parallel dependencies.

Threads are currently only implemented fully on Windows, but the infrastructure I've created should also work on Linux (as pthread is very simple).

The Profiler itself

The profiler has a simple interface; instantiate a Profiler with a name to start a block with that name, and when it goes out of scope, that block ends.

A block is to be represented as an actual rectangle on a graph, with width proportional to the time spent in the block.

There are blocks for all kinds of events; see the EventType enum:

    enum class EventType {
      Begin,
      Color,
      End,
      Frame,
      String,
      Int,
      FiberWait,
      FiberWake,
      ContextSwitch,
      Job,
      GPUBegin,
      GPUEnd,
      Link,
      Pause,
      GPUStats,
      Continue,
      Signal,
      Counter
    };

The Profiler is capable of linking to and profiling GPU processes, via the GPUBegin and Link functions.
Other functions are provided for utility, such as color, string, int, frame.

Profilers are per-thread; any calls to a Profiler function will only apply to the profiler for the thread that called the function, even if you call across threads; always the calling thread is updated.

This allows multiple parallel block diagrams to represent what any number of threads are doing at any given time.

The profiler is currently only implemented for Windows, due to extensively using the Windows thread trace functions.

Closes #19.

dpeter99

Looks nice, the thread support does add a big pile of complexity for sure.
Currently on Linux so can't test it.
I would prefer if the core had no imports from windows, so it would be nice if the profiler stuff was abstracted out to the platforms folder as well.

I have a vision where everything you need to run shadow on a new platform was in that single folder and if something was missing it was clear.

dpeter99 · 2024-04-05T17:41:36Z

projs/shadow/shadow-engine/core/inc/shadow/core/Thread.h

+    virtual int Run() = 0;
+
+    // Start the thread and start it running.
+    bool Start(const char* name, bool extended);


Add proper comments on what the name and extended are used for.

The name is the name of the thread. I'm not sure how that can be explained.

Extended exists for something I'm planning later, currently unused.

could you add those details as proper doc comments?
Would remove the guess work

dpeter99 · 2024-04-05T17:44:27Z

projs/shadow/shadow-engine/core/inc/shadow/core/Time.h

+        return elapsed;
+    }
+
+    static size_t getTimestamp();


Why is this size_t while the others are double?
Guessing this is the raw unix timestamp or similar, but a comment wouldn't hurt.

It's a raw metric.

The others are in seconds/milliseconds, with high enough precision to have fractional parts (milliseconds/nanoseconds), but this one is directly from the OS, and both Linux and Windows return a size_t here. One is a Unix timestamp, one is a processor tick counter.

It's platform specific, so it's difficult to explain in the common section.

If it is a platform depended thing and not even the same time scale than it needs a proper comment even more

TheCurle · 2024-04-06T18:37:24Z

An example usage of the Profiler counter, for tracking process memory usage:

        const float memory = Platform::GetProcessMemory() / (1024 * 1024);
        static size_t processMemoryCounter = SH::Profiler::MakeCounter("Process Memory (MB) ", 0);
        SH::Profiler::PushCounter(processMemoryCounter, memory);

This must be done on the main thread, and from that point on, the main thread will have a Process Memory usage counter attached. If this block of code is called every frame, the memory usage will be updated every frame, and a graph chart will be produced showing how the memory changes over time.

dpeter99

The Mutex and condition classes are missing.
The Profiler's platform specific stuff should be moved so we can support many targets. Adding increasing amounts of ifdefs is gonna be unmanagable.

projs/shadow/shadow-engine/platforms/windows/src/Thread.cpp

projs/shadow/shadow-engine/core/src/profile/Profiler.cpp

dpeter99 · 2024-04-07T15:22:19Z

projs/shadow/shadow-engine/core/src/profile/Profiler.cpp

+    size_t threadID;
+  };
+
+  // This ifdef contains a bunch of the infrastructure necessary for performance monitoring under Windows.


This section should be extracted to the platforms folder so I can even try to add Linux support and later of course for the console support

Fair point, though if i do that i'll have to duplicate the entire code between Linux and Windows, because these are static internal classes, not exposed in the header.

It's here for the DRY principle. A lot would be repeated if these were in their own file.

projs/shadow/shadow-engine/core/src/profile/Profiler.cpp

TheCurle · 2024-04-07T15:36:06Z

I've pushed the files from the other branch that this PR depends on.

dpeter99

This is only a first look-over, will do another focused on the actual meat of the PR.
Feels like a good direction. Maybe we should
a) make the file stuff a separate PR and delay the development more
b) add those files to the PR description and title

dpeter99 · 2024-04-10T09:10:40Z

projs/shadow/shadow-engine/assets/inc/shadow/assets/fs/file.h

+namespace ShadowEngine {
+
+    // An input stream that can read a file on disk.
+    struct FileInput final : InputStream {


Why is the file system needed here?

dpeter99 · 2024-04-10T09:26:50Z

projs/shadow/shadow-engine/assets/inc/shadow/assets/Mesh.h

-        //The indices of the mesh
-        std::vector<uint32_t> indices;
-    };
+#pragma once


Could you figure out what is up with this file?
maybe line endings or something?

dpeter99 · 2024-04-10T13:28:53Z

projs/shadow/shadow-engine/assets/src/Mesh.cpp

Why changes here? 🫨

dpeter99 · 2024-04-10T13:30:13Z

projs/shadow/shadow-engine/core/inc/shadow/core/Thread.h

+    virtual int Run() = 0;
+
+    // Start the thread and start it running.
+    bool Start(const char* name, bool extended);


could you add those details as proper doc comments?
Would remove the guess work

dpeter99 reviewed Apr 5, 2024

View reviewed changes

TheCurle mentioned this pull request Apr 6, 2024

Asset management system #31

Draft

dpeter99 requested changes Apr 7, 2024

View reviewed changes

TheCurle added 5 commits April 7, 2024 19:40

Fix setup

41ce4e4

Add Platform-specific Threads, and the Profiler interface.

6890f29

Finish Profiler, update Time with platform-specific precision method

0449b1b

Add missing Common header

7430270

Add filesystem support from the assets branch

046b1e2

TheCurle force-pushed the curle/profiler branch from 79121cf to 046b1e2 Compare April 7, 2024 18:40

Remove unnecessary changes to main branch

10abaec

TheCurle force-pushed the curle/profiler branch from 353acec to 10abaec Compare April 7, 2024 20:42

dpeter99 marked this pull request as draft April 8, 2024 21:59

Split profiler into platform variant files

8a1b17e

dpeter99 reviewed Apr 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a profiler and instrumentation #30

Add a profiler and instrumentation #30

TheCurle commented Apr 5, 2024 •

edited

Loading

dpeter99 left a comment

dpeter99 Apr 5, 2024

TheCurle Apr 5, 2024

dpeter99 Apr 10, 2024

dpeter99 Apr 5, 2024

TheCurle Apr 5, 2024

dpeter99 Apr 7, 2024

TheCurle commented Apr 6, 2024

dpeter99 left a comment

dpeter99 Apr 7, 2024

TheCurle Apr 7, 2024

TheCurle commented Apr 7, 2024

dpeter99 left a comment •

edited

Loading

dpeter99 Apr 10, 2024

dpeter99 Apr 10, 2024

dpeter99 Apr 10, 2024

dpeter99 Apr 10, 2024

Add a profiler and instrumentation #30

Are you sure you want to change the base?

Add a profiler and instrumentation #30

Conversation

TheCurle commented Apr 5, 2024 • edited Loading

Precise Timer

Threads

The Profiler itself

dpeter99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheCurle commented Apr 6, 2024

dpeter99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheCurle commented Apr 7, 2024

dpeter99 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheCurle commented Apr 5, 2024 •

edited

Loading

dpeter99 left a comment •

edited

Loading