Instrument jl_gc_big_alloc() when called from generated code for Allocations Profiler #44043

NHDaly · 2022-02-04T18:51:11Z

Instrument jl_gc_big_alloc() when called from generated code for Allocations Profiler

Add _maybe_record_alloc_to_profile() call into jl_gc_big_alloc() to
allow us to instrument it in the Allocations Profiler.

Followup to #43868

Finishes last case of #43688 (gets the stacks, though not the types, of big allocs).

Before

*: instrumented; red: currently missed; green: newly introduced function

graphviz

digraph G {
  node [shape=box];

  generated_code [label="<generated code>"];

  new_array;
  jl_gc_alloc_string [label="jl_gc_alloc_string *"];
  jl_gc_alloc [label="jl_gc_alloc *"];
  jl_gc_managed_malloc [label="jl_managed_malloc *"]; // https://github.com/vilterp/julia/pull/20
  jl_gc_pool_alloc [fillcolor="#90ed8f" style=filled label="jl_gc_pool_alloc *"];
  jl_gc_pool_alloc_noinline [fillcolor="#90ed8f" style=filled label="jl_gc_pool_alloc_noinline"];
  jl_gc_pool_alloc_inner;
  jl_gc_big_alloc;

  {
    rank=same;
    jl_gc_managed_malloc;
    jl_gc_pool_alloc_noinline;
    jl_gc_big_alloc;
  }

  generated_code -> jl_gc_alloc_string;
  generated_code -> new_array;
  generated_code -> jl_gc_alloc;
  generated_code -> jl_gc_big_alloc [color=red];
  generated_code -> jl_gc_pool_alloc;
  jl_gc_pool_alloc -> jl_gc_pool_alloc_inner  [style=dotted label="inlined"];
  jl_gc_pool_alloc_noinline -> jl_gc_pool_alloc_inner  [style=dotted  label="inlined"];


  jl_gc_alloc -> jl_gc_pool_alloc_noinline;
  jl_gc_alloc -> jl_gc_big_alloc;
  new_array -> jl_gc_alloc;
  new_array -> jl_gc_managed_malloc;
  jl_gc_alloc_string -> jl_gc_pool_alloc_noinline;
  jl_gc_alloc_string -> jl_gc_big_alloc;
}

After

*: instrumented; green: newly introduced function

graphviz

digraph G {
  node [shape=box];

  generated_code [label="<generated code>"];

  new_array;
  jl_gc_alloc_string [label="jl_gc_alloc_string *"];
  jl_gc_alloc [label="jl_gc_alloc *"];
  jl_gc_managed_malloc [label="jl_managed_malloc *"]; // https://github.com/vilterp/julia/pull/20
  jl_gc_pool_alloc [fillcolor="#90ed8f" style=filled label="jl_gc_pool_alloc *"];
  jl_gc_pool_alloc_noinline [fillcolor="#90ed8f" style=filled label="jl_gc_pool_alloc_noinline"];
  jl_gc_pool_alloc_inner;
  jl_gc_big_alloc [fillcolor="#90ed8f" style=filled label="jl_gc_big_alloc *"];
  jl_gc_big_alloc_noinline [fillcolor="#90ed8f" style=filled label="jl_gc_pool_alloc_noinline"];
  jl_gc_big_alloc_inner;


  {
    rank=same;
    jl_gc_managed_malloc;
    jl_gc_pool_alloc_noinline;
    jl_gc_big_alloc_noinline;
  }

  generated_code -> jl_gc_alloc_string;
  generated_code -> new_array;
  generated_code -> jl_gc_alloc;
  generated_code -> jl_gc_big_alloc;
  generated_code -> jl_gc_pool_alloc;
  jl_gc_pool_alloc -> jl_gc_pool_alloc_inner  [style=dotted label="inlined"];
  jl_gc_pool_alloc_noinline -> jl_gc_pool_alloc_inner  [style=dotted  label="inlined"];
  jl_gc_big_alloc -> jl_gc_big_alloc_inner  [style=dotted label="inlined"];
  jl_gc_big_alloc_noinline -> jl_gc_big_alloc_inner  [style=dotted  label="inlined"];


  jl_gc_alloc -> jl_gc_pool_alloc_noinline;
  jl_gc_alloc -> jl_gc_big_alloc_noinline;
  new_array -> jl_gc_alloc;
  new_array -> jl_gc_managed_malloc;
  jl_gc_alloc_string -> jl_gc_pool_alloc_noinline;
  jl_gc_alloc_string -> jl_gc_big_alloc_noinline;
}

vilterp

this looks in line (pun somewhat intended) with what we do for pool alloc :)

NHDaly · 2022-02-04T19:06:28Z

@nanosoldier runbenchmarks("alloc", vs = ":pv-alloc-profile-wrap-pool-alloc")

nanosoldier · 2022-02-04T19:19:06Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

vilterp · 2022-02-04T19:43:06Z

^ Kind of hard to believe this isn't just noise… Like big alloc gets called waaay less than pool alloc — how could instrumenting pool alloc not cause a regression, but this does? 🤔

NHDaly · 2022-02-04T19:58:03Z

Yep, 100% agreed. The Nanosoldier report for the pool_alloc PR showed a 5% improvement, which is almost certainly impossible, and this one shows a 5% regression. So i think both are almost certainly lost in the noise 👍 👍

src/gc.c

…cations Profiler Add _maybe_record_alloc_to_profile() call into jl_gc_big_alloc() to allow us to instrument it in the Allocations Profiler. Followup to #43868 Finishes fixing #43688 (gets the stacks, though not the types, of big allocs). ----- - Rename jl_gc_big_alloc to jl_gc_big_alloc_inner, make it inlined - add jl_gc_pool_alloc_noinline that calls jl_gc_big_alloc_inner - replace all internal calls to jl_gc_big_alloc to call this instead - add a new jl_gc_pool_alloc that calls jl_gc_big_alloc_inner - This one is instrumented, and is meant to be called by generated code.

…44066)

NHDaly · 2022-02-07T20:49:58Z

K, I've also updated the diagrams at the top of the PR description. This should be ready for a re-review @vtjnash. Thanks again! 👍

src/julia_internal.h

@vchuravy

Dropping JL_DLLEXPORT from jl_gc_big_alloc_noinline declaration, per @vchuravy's previous comment, above.

NHDaly · 2022-02-08T14:52:52Z

Thanks everyone! Merging now 👍

vchuravy · 2022-02-09T01:43:45Z

src/julia_internal.h

@@ -231,7 +231,7 @@ JL_DLLEXPORT extern const char *jl_filename;

 jl_value_t *jl_gc_pool_alloc_noinline(jl_ptls_t ptls, int pool_offset,
                                   int osize);
-JL_DLLEXPORT jl_value_t *jl_gc_big_alloc(jl_ptls_t ptls, size_t allocsz);


I think you technically want to keep this line still? jl_gc_big_alloc is still an exported function?

🤔 it is, but it's not meant to be called anywhere from inside our C code, only from LLVM-generated code. So it doesn't need to be declared in julia_internal.h, which, from what i understand, is a private header only for internal functions?

It's still exported via the definition, which is all that really matters to the linker anyway, this declaration is only here for compiling C files.

So i think it's okay to delete, like we did. We did the same thing for pool_alloc in #43688, too.

…cations Profiler (#44043) * Instrument jl_gc_big_alloc() when called from generated code for Allocations Profiler Add _maybe_record_alloc_to_profile() call into jl_gc_big_alloc() to allow us to instrument it in the Allocations Profiler. Followup to #43868 Finishes fixing #43688 (gets the stacks, though not the types, of big allocs). ----- - Rename jl_gc_big_alloc to jl_gc_big_alloc_inner, make it inlined - add jl_gc_pool_alloc_noinline that calls jl_gc_big_alloc_inner - replace all internal calls to jl_gc_big_alloc to call this instead - add a new jl_gc_pool_alloc that calls jl_gc_big_alloc_inner - This one is instrumented, and is meant to be called by generated code. Co-authored-by: Pete Vilter <[email protected]>

…cations Profiler (JuliaLang#44043) * Instrument jl_gc_big_alloc() when called from generated code for Allocations Profiler Add _maybe_record_alloc_to_profile() call into jl_gc_big_alloc() to allow us to instrument it in the Allocations Profiler. Followup to JuliaLang#43868 Finishes fixing JuliaLang#43688 (gets the stacks, though not the types, of big allocs). ----- - Rename jl_gc_big_alloc to jl_gc_big_alloc_inner, make it inlined - add jl_gc_pool_alloc_noinline that calls jl_gc_big_alloc_inner - replace all internal calls to jl_gc_big_alloc to call this instead - add a new jl_gc_pool_alloc that calls jl_gc_big_alloc_inner - This one is instrumented, and is meant to be called by generated code. Co-authored-by: Pete Vilter <[email protected]>

…cations Profiler (#44043) * Instrument jl_gc_big_alloc() when called from generated code for Allocations Profiler Add _maybe_record_alloc_to_profile() call into jl_gc_big_alloc() to allow us to instrument it in the Allocations Profiler. Followup to #43868 Finishes fixing #43688 (gets the stacks, though not the types, of big allocs). ----- - Rename jl_gc_big_alloc to jl_gc_big_alloc_inner, make it inlined - add jl_gc_pool_alloc_noinline that calls jl_gc_big_alloc_inner - replace all internal calls to jl_gc_big_alloc to call this instead - add a new jl_gc_pool_alloc that calls jl_gc_big_alloc_inner - This one is instrumented, and is meant to be called by generated code. Co-authored-by: Pete Vilter <[email protected]>

NHDaly changed the base branch from master to pv-alloc-profile-wrap-pool-alloc February 4, 2022 18:57

NHDaly requested a review from vchuravy February 4, 2022 18:58

vilterp approved these changes Feb 4, 2022

View reviewed changes

NHDaly marked this pull request as ready for review February 4, 2022 19:05

vchuravy approved these changes Feb 4, 2022

View reviewed changes

vtjnash reviewed Feb 7, 2022

View reviewed changes

src/gc.c Show resolved Hide resolved

NHDaly force-pushed the nhd-alloc-profile-wrap-big-alloc branch from 4da776a to 4d0fd4d Compare February 7, 2022 17:28

NHDaly changed the base branch from pv-alloc-profile-wrap-pool-alloc to master February 7, 2022 17:28

NHDaly mentioned this pull request Feb 7, 2022

allocs profiler: extend to record Types for all allocations - (fix UnknownType in allocations results) #43688

Closed

vilterp mentioned this pull request Feb 7, 2022

alloc profiler: fix use of pool_alloc_noinline where big_alloc_noinline was intended #44066

Merged

fix use of pool_alloc_noinline where big_alloc_noinline was intended (#…

7f265d0

…44066)

NHDaly requested a review from vtjnash February 7, 2022 20:50

vtjnash approved these changes Feb 8, 2022

View reviewed changes

vchuravy reviewed Feb 8, 2022

View reviewed changes

src/julia_internal.h Show resolved Hide resolved

NHDaly commented Feb 8, 2022

View reviewed changes

src/julia_internal.h Outdated Show resolved Hide resolved

Update src/julia_internal.h - PR review feedback

58b036d

Dropping JL_DLLEXPORT from jl_gc_big_alloc_noinline declaration, per @vchuravy's previous comment, above.

NHDaly merged commit 24d96ab into master Feb 8, 2022

NHDaly deleted the nhd-alloc-profile-wrap-big-alloc branch February 8, 2022 14:53

vchuravy reviewed Feb 9, 2022

View reviewed changes

NHDaly mentioned this pull request Feb 10, 2022

[scratch] Scratch PR for backporting the allocations profiler to julia 1.7 (we're targeting this branch in our build at RelationalAI) #44119

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instrument jl_gc_big_alloc() when called from generated code for Allocations Profiler #44043

Instrument jl_gc_big_alloc() when called from generated code for Allocations Profiler #44043

NHDaly commented Feb 4, 2022 •

edited

Loading

vilterp left a comment

NHDaly commented Feb 4, 2022

nanosoldier commented Feb 4, 2022

vilterp commented Feb 4, 2022 •

edited

Loading

NHDaly commented Feb 4, 2022

NHDaly commented Feb 7, 2022

NHDaly commented Feb 8, 2022

vchuravy Feb 9, 2022

NHDaly Feb 18, 2022

Instrument jl_gc_big_alloc() when called from generated code for Allocations Profiler #44043

Instrument jl_gc_big_alloc() when called from generated code for Allocations Profiler #44043

Conversation

NHDaly commented Feb 4, 2022 • edited Loading

Before

After

vilterp left a comment

Choose a reason for hiding this comment

NHDaly commented Feb 4, 2022

nanosoldier commented Feb 4, 2022

vilterp commented Feb 4, 2022 • edited Loading

NHDaly commented Feb 4, 2022

NHDaly commented Feb 7, 2022

NHDaly commented Feb 8, 2022

vchuravy Feb 9, 2022

Choose a reason for hiding this comment

NHDaly Feb 18, 2022

Choose a reason for hiding this comment

NHDaly commented Feb 4, 2022 •

edited

Loading

vilterp commented Feb 4, 2022 •

edited

Loading