Feature: mark a Func as no_profiling, to prevent injection of profiling. (2nd implementation) by mcourteaux · Pull Request #8143 · halide/Halide

mcourteaux · 2024-03-07T13:02:51Z

Replaces #8136 with a cleaner implementation.

@abadams I took your suggestion, and this implementation is much better. Very small changes, and the issue I reported in the previous one is gone. The trick with the "environment" was a good lesson!

Additionally, I managed to also clean up the report by having the stack-allocated Allocate/Free nodes also have their allocation contribute to the enclosing Func. Doing that changed my profiling report from (w_0, w_1, w_2, max_channel, lum, lum_pow3, r, g, b all marked with no_profiling(), but still producing lines in the report):

neonraw_highlight_ratio_extractor
 total time: 1338.695068 ms  samples: 1262  runs: 48  time/run: 27.889481 ms
 average threads used: 2.657686
 heap allocations: 0  peak heap usage: 0 bytes
    halide_malloc:                0.000ms ( 0.0%)   threads: 0.000 
    halide_free:                  0.000ms ( 0.0%)   threads: 0.000 
    ratio_map:                   21.282ms (76.3%)   threads: 2.663 
    repeat_edge:                  2.764ms ( 9.9%)   threads: 3.248  stack: 3960
    max_field_hpass:              0.683ms ( 2.4%)   threads: 2.967  stack: 3840
    max_field:                    0.352ms ( 1.2%)   threads: 2.125  stack: 3072
    min_field_hpass:              0.507ms ( 1.8%)   threads: 2.826  stack: 1280
    max_channel:                  0.000ms ( 0.0%)   threads: 0.000  stack: 132
    min_field:                    0.088ms ( 0.3%)   threads: 1.250  stack: 1024
    b:                            0.000ms ( 0.0%)   threads: 0.000  stack: 64
    g:                            0.000ms ( 0.0%)   threads: 0.000  stack: 64
    r:                            0.000ms ( 0.0%)   threads: 0.000  stack: 64
    lum:                          0.000ms ( 0.0%)   threads: 0.000  stack: 64
    lum_pow3:                     0.000ms ( 0.0%)   threads: 0.000  stack: 64
    lum4:                         2.210ms ( 7.9%)   threads: 1.870  stack: 64
    w_0:                          0.000ms ( 0.0%)   threads: 0.000  stack: 64
    w_1:                          0.000ms ( 0.0%)   threads: 0.000  stack: 64
    w_2:                          0.000ms ( 0.0%)   threads: 0.000  stack: 64

to this (note how the stack size contributions (132 + 8 * 64 = 644) moved up into the enclosing Funcs:

min_field_hpass: 1412 - 1280 = 132 bytes extra,
ratio_map 384 - 0 = 384 bytes extra

The total difference is less than 644 bytes, because the lifetimes of some of the variables were actually fully-disjunct, causing them to not actually have a peak stack usage of that much.):

neonraw_highlight_ratio_extractor
 total time: 1524.809326 ms  samples: 1416  runs: 48  time/run: 31.766861 ms
 average threads used: 4.375706
 heap allocations: 0  peak heap usage: 0 bytes
    halide_malloc:                0.000ms ( 0.0%)   threads: 0.000 
    halide_free:                  0.000ms ( 0.0%)   threads: 0.000 
    ratio_map:                   24.556ms (77.3%)   threads: 4.067  stack: 384
    repeat_edge:                  3.719ms (11.7%)   threads: 5.716  stack: 3960
    max_field_hpass:              0.420ms ( 1.3%)   threads: 3.000  stack: 3840
    max_field:                    0.336ms ( 1.0%)   threads: 5.666  stack: 3072
    min_field_hpass:              0.575ms ( 1.8%)   threads: 3.846  stack: 1412
    min_field:                    0.088ms ( 0.2%)   threads: 8.500  stack: 1024
    lum4:                         2.069ms ( 6.5%)   threads: 5.813  stack: 64

…oop) Func should not be profiled.

abadams · 2024-03-07T19:01:23Z

+        }
+    }
+
+    const Function *lookup_function(const string &name) const {


Function is already a pointer type, so this should probably just return a Function by value. Alternatively you could return a const Function &, but I'm not sure how to satisfy the compiler in the case where you need to return something valid after the internal_error. Maybe just return env.begin()->second?

I first had a reference, but I in case the lookup fails, I need to satisfy the compiler and give it a nullptr. Idk if we can easily create a nullptr-Function& instead of a Function* being nullptr?

I suggest satisfying the compiler by returning the first Function in the environment: env.begin()->second.

There is a default constructor I see. So for this unreachable code, a simple return {}; looks a bit nicer I think?

Isn't that returning a reference to a local? Given that it's unreachable, whatever makes the compiler shut up is fine.

Isn't that returning a reference to a local?

AFAICT, no. It's invoking the default constructor (the one literally defined with = default;), which just initialized the FunctionPtr inside to be default-constructed, meaning that there are just some pointers put to nullptr I assume.

abadams · 2024-03-07T19:01:59Z

    trace_stores: bool = false;
    trace_realizations: bool = false;
    trace_tags: [string];
+    no_profiling: bool = false;


Probably need to bump the serialization minor version number

I'm actually clueless where to find that number.

I'm a bit confused too. @TH3CHARLie ?

it gets refactored earlier to here:

Halide/src/halide_ir.fbs

Line 9 in 8cc4f02

enum SerializationVersionMajor: int {

@TH3CHARLie looking at those inline comments, suggest that I shouldn't touch anything, as we are still at "unstable"? Do I bump the patch version instead?

yeah I believe so

@abadams @TH3CHARLie It all looks suspicious, as the Patch version was at 0 (zero). Serialization version 18.0.0 seems suspicious, but maybe is what we are after? I just bumped it to 18.0.1.

It was at zero since we recently had a release and introduced this version numbering system and hasn't added anything new to the format, I don't understand what's suspicious here.

Makes sense! Thanks!

…of the serialization.

mcourteaux · 2024-03-12T21:24:43Z

AFAICT the failing build is unrelated, right? This might conflict with abadams/per_instance_profiling, in which case I'm happy to adapt this PR after we merge in Andrew's work.

abadams · 2024-03-12T23:56:34Z

Yes, the failure is unrelated (fixed by #8148)

mcourteaux added 3 commits March 7, 2024 13:27

Small feature to allow you to specify that a (typically small inner l…

d1ca091

…oop) Func should not be profiled.

Simplified the tuple name handling.

e3643c0

Optimize tuple name normalization in Profiling.cpp

d011e43

mcourteaux changed the title ~~Small feature to allow you to specify that a (typically small inner loop) Func should not be profiled.~~ Feature: mark a Func as no_profiling, to prevent injection of profiling. Mar 7, 2024

mcourteaux changed the title ~~Feature: mark a Func as no_profiling, to prevent injection of profiling.~~ Feature: mark a Func as no_profiling, to prevent injection of profiling. (2nd implementation) Mar 7, 2024

mcourteaux mentioned this pull request Mar 7, 2024

Feature: mark a Func as no_profiling, to prevent injection of profiling. #8136

Closed

Clang-format

25f9990

mcourteaux force-pushed the func-no-profile-2 branch from c030bb1 to 25f9990 Compare March 7, 2024 13:23

steven-johnson requested a review from abadams March 7, 2024 17:23

abadams reviewed Mar 7, 2024

View reviewed changes

Feedback on Function already being a pointer. Bump the Patch version …

b8bda38

…of the serialization.

abadams requested a review from halidebuildbots March 9, 2024 22:33

abadams approved these changes Mar 12, 2024

View reviewed changes

abadams merged commit 4988ab5 into halide:main Mar 12, 2024

abadams added the release_notes For changes that may warrant a note in README for official releases. label Mar 13, 2024

BrewTestBot mentioned this pull request Jul 17, 2024

halide 18.0.0 Homebrew/homebrew-core#177657

Closed

1 task

Conversation

mcourteaux commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

mcourteaux Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mcourteaux commented Mar 12, 2024

Uh oh!

abadams commented Mar 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mcourteaux commented Mar 7, 2024 •

edited

Loading

mcourteaux Mar 8, 2024 •

edited

Loading