Add profiling instrumentation for NAM building blocks by jfsantos · Pull Request #219 · sdatkinson/NeuralAmpModelerCore

jfsantos · 2026-02-06T18:07:54Z

Adds a profiling framework (NAM/profiling.h, NAM/profiling.cpp) with NAM_PROFILE_START()/NAM_PROFILE_ADD() macros and 14 timing categories. Supports both desktop (std::chrono) and ARM Cortex-M7 (DWT cycle counter) backends. Profiling is compile-time gated via -DNAM_PROFILING.

Instruments wavenet _Layer::Process() and _LayerArray::ProcessInner() with per-category timing, and adds profiling reset/print calls to the benchmodel tool.

Developed with support and sponsorship from TONE3000

Adds a profiling framework (NAM/profiling.h, NAM/profiling.cpp) with NAM_PROFILE_START()/NAM_PROFILE_ADD() macros and 14 timing categories. Supports both desktop (std::chrono) and ARM Cortex-M7 (DWT cycle counter) backends. Profiling is compile-time gated via -DNAM_PROFILING. Instruments wavenet _Layer::Process() and _LayerArray::ProcessInner() with per-category timing, and adds profiling reset/print calls to the benchmodel tool. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

sdatkinson

Some comments & Q's.

The main thing that makes me pause is adding things into the high-level logic like in the Layer process method. If there were a way to do it that's a little more "hidden", then that would be much preferred before merging this into main...

sdatkinson · 2026-02-12T00:45:41Z

NAM/conv1d.cpp

@@ -1,4 +1,5 @@
 #include "conv1d.h"
+#include "profiling.h"


Does this have to be included? Feels weird to include it if I'm doing e.g. a release build.

Fair, it should at very least be inside an #ifdef. I'll change that.

sdatkinson · 2026-02-12T00:47:45Z

NAM/profiling.h

+  }
+
+  uint32_t total() const {
+    return conv1d + input_mixin + layer1x1 + head1x1 + rechannel + conv1x1 + activation + film + copies + setzero + ringbuf + condition + lstm + other;


This feels fragile. Is there a way to "register" things or something?

This whole implementation was targeted at the Wavenet and I didn't think of generalizing it, only covered anything that will be in A2... but you are right, it would be better if we just kept a dictionary of things we are measuring and reported all of them, just like a logging library.

sdatkinson · 2026-02-12T00:48:29Z

NAM/profiling.h

+    }
+  };
+
+  print_row("Conv1D", t.conv1d);


Yeah...this could be an iterator over the various registered ops?

sdatkinson · 2026-02-12T00:51:52Z

NAM/wavenet.cpp


 void nam::wavenet::_Layer::Process(const Eigen::MatrixXf& input, const Eigen::MatrixXf& condition, const int num_frames)
 {
+  NAM_PROFILE_START();


Yeah...I'm not super-hot on things like this sitting in the middle of the code. Is there a way to do this that doesn't add lines like this?

sdatkinson · 2026-02-12T00:54:11Z

tools/CMakeLists.txt

 # Also ensure assertions are enabled (NDEBUG is not defined) so tests actually run
 set_target_properties(run_tests PROPERTIES COMPILE_OPTIONS "-O0")
+# Benchmodel should be built with NAM_PROFILING set
+target_compile_definitions(benchmodel PRIVATE NAM_PROFILING)


Is there a way to make this optional via a command-line flag?

I'd also be interested to see how much overhead the profiler adds.

I'll look into how to make it a CMake flag. I briefly looked into it, thought it was more complicated than I wanted to deal with, and immediately forgot about it... especially since profiling should only be inserted into profiling tools.

sdatkinson · 2026-02-12T00:56:42Z

tools/memory_usage.cpp

+// Conv1x1 stores either a full (out_channels x in_channels) matrix (possibly
+// block-diagonal when grouped), or a depthwise weight vector when groups ==
+// in_channels == out_channels.
+static MemoryResult conv1x1_memory(int in_ch, int out_ch, bool bias, int groups, int M)


So this stores only the memory due to weights? Is there any overhead that should be accounted for? It's probably small but I figure I should ask while I'm here.

No overhead for small local variables, but if anything is allocating an entire temporary buffer that wasn't accounted for we need to take that into account. Internal buffers are being accounted for (see e.g. line 59)

sdatkinson · 2026-02-12T00:57:35Z

tools/memory_usage.cpp

+
+// Conv1D stores kernel_size weight matrices (each out_ch x in_ch) or depthwise
+// vectors, plus a bias vector, a ring buffer, and an output buffer.
+static MemoryResult conv1d_memory(int in_ch, int out_ch, int kernel_size, bool bias, int dilation, int groups, int M)


What's M?

M is buffer_size everywhere, thanks for catching it. I'll rename it all across the file.

sdatkinson · 2026-02-12T01:04:17Z

tools/memory_usage.cpp

+  // Recursive condition_dsp
+  bool has_condition_dsp = false;
+  int condition_output_channels = condition_dim;
+  if (config.find("condition_dsp") != config.end())


Should check for null like #220 ?

That's correct, thanks for catching it!

…son#220)

…lementation (sdatkinson#223) * Make activation apply method pure virtual instead of no-op default * Fix bugs * Refactor to throw std::invalid_argument in debug mode, add tests

The Timings struct hardcoded 14 named fields, requiring manual updates to reset(), total(), print_results(), and every call site whenever a category was added or removed. Replace with a flat-array registry where types are registered at file scope via register_type(), returning an integer index for O(1) accumulation in the hot path. Also adds NAM_PROFILE_RESTART() macro to replace a raw #ifdef block in wavenet.cpp.

sdatkinson

The code is nice as it is. The trouble with it, like I've said elsewhere, is that I feel uneasy about having this "in main".

What I'm going to do in the meantime is make a main-profiling branch and merge this into there.

It'll be a bit more work to keep the two in sync, but if this really seems to be worth the lines (or I get other feedback from folks saying that they like it) then I'll merge it up to main later.

João Felipe Santos and others added 3 commits February 6, 2026 09:33

Fixed build flags for benchmodel

5c53524

Added a command line tool to output memory usage for a given .nam file

84deb8a

sdatkinson requested changes Feb 12, 2026

View reviewed changes

jfsantos and others added 4 commits February 12, 2026 11:17

Bugfix - checking that condition_dsp is not null in the JSON (sdatkin…

725c8ca

…son#220)

[BUGFIX, BREAKING] Make activation base class abstract, fix PReLU imp…

c956055

…lementation (sdatkinson#223) * Make activation apply method pure virtual instead of no-op default * Fix bugs * Refactor to throw std::invalid_argument in debug mode, add tests

Add TONE3000 support note in README.md (sdatkinson#224)

d499f74

sdatkinson reviewed Feb 14, 2026

View reviewed changes

sdatkinson changed the base branch from main to main-profiling February 14, 2026 00:54

sdatkinson merged commit 3e4bc05 into sdatkinson:main-profiling Feb 14, 2026
2 checks passed

sdatkinson mentioned this pull request Feb 14, 2026

Update README with tools for library usage #229

Merged

Conversation

jfsantos commented Feb 6, 2026

Uh oh!

sdatkinson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdatkinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants