Instrument jl_gc_big_alloc() when called from generated code for Allocations Profiler by NHDaly · Pull Request #44043 · JuliaLang/julia

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


 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 <7341+vilterp@users.noreply.github.com>

…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 <7341+vilterp@users.noreply.github.com>

…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 <7341+vilterp@users.noreply.github.com>

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

Uh oh!

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 merged 3 commits intomasterfrom
nhd-alloc-profile-wrap-big-alloc

NHDaly commented Feb 4, 2022 •

edited

Loading

Uh oh!

vilterp left a comment

Uh oh!

NHDaly commented Feb 4, 2022

Uh oh!

nanosoldier commented Feb 4, 2022

Uh oh!

vilterp commented Feb 4, 2022 •

edited

Loading

Uh oh!

NHDaly commented Feb 4, 2022

Uh oh!

Uh oh!

NHDaly commented Feb 7, 2022

Uh oh!

Uh oh!

Uh oh!

NHDaly commented Feb 8, 2022

Uh oh!

vchuravy Feb 9, 2022

Uh oh!

NHDaly Feb 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

NHDaly commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before

After

Uh oh!

vilterp left a comment

Choose a reason for hiding this comment

Uh oh!

NHDaly commented Feb 4, 2022

Uh oh!

nanosoldier commented Feb 4, 2022

Uh oh!

vilterp commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NHDaly commented Feb 4, 2022

Uh oh!

Uh oh!

NHDaly commented Feb 7, 2022

Uh oh!

Uh oh!

Uh oh!

NHDaly commented Feb 8, 2022

Uh oh!

vchuravy Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

NHDaly Feb 18, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

NHDaly commented Feb 4, 2022 •

edited

Loading

vilterp commented Feb 4, 2022 •

edited

Loading