801775 - IonMonkey: Filter logging spew based on script's origin

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

Attachment: Filter logging spew based on script origin.

Add an environment variable to enable C1/Ion spewer only when the filter match the origin of the script.  This is useful to track reasons of bailouts in large scripts, such as v8, octane and maybe browsing sessions.

Such as Ion spew of bug 799818 can be filtered to produce less a few compilation results instead of hundreds:

IONFILTER=pdfjs.js:17068,pdfjs.js:26758 IONFLAGS=filter,logs ./dbg/js …

Comment 1

[Sean Stangl [:sstangl]]

Comment on attachment 671522 [details] [diff] [review]
Filter logging spew based on script origin.

Review of attachment 671522 [details] [diff] [review]:
-----------------------------------------------------------------

Needs a comment explaining the format of IONFILTER, with some examples. Since this patch is debug-only, it is OK.

::: js/src/ion/IonSpewer.cpp
@@ +35,5 @@
>  #undef IONSPEW_CHANNEL
>  };
>  
> +static bool
> +ContainsLocation(const char *filename, const size_t line = size_t(-1))

FilterContainsLocation()

@@ +45,5 @@
> +    static const char *filter = getenv("IONFILTER");
> +    static size_t filelen = strlen(filename);
> +    const char *index = strstr(filter, filename);
> +    while (index) {
> +        if (index == filter || index[-1] == ',') {

This logic won't work for files with commas in their names, but I guess that's OK.

@@ +70,5 @@
>  ion::IonSpewNewFunction(MIRGraph *graph, JSScript *function)
>  {
>      if (!js_IonOptions.parallelCompilation)
> +        if (ContainsLocation(function->filename, function->lineno))
> +            ionspewer.beginFunction(graph, function);

Outer if() needs braces, but it might looks nicer to just invert the condition on js_IonOptions.parallelCompilation and return early.

It might be better to hide the ContainsLocation() logic in beginFunction(), so that we don't accidentally call the latter.

@@ +122,5 @@
>  
> +bool
> +IonSpewer::enabled()
> +{
> +    return inited_ && graph && function;

Can graph be set but function unset?

@@ +269,5 @@
>          EnableChannel(IonSpew_Pools);
>      if (ContainsFlag(env, "logs"))
>          EnableIonDebugLogging();
> +    if (ContainsFlag(env, "filter"))
> +        FilterLogs = true;

This seems unnecessary -- why not just detect whether the envvar IONFILTER is set, and use it if it is?

::: js/src/ion/IonSpewer.h
@@ +86,5 @@
>      ~IonSpewer();
>  
>      bool init();
>      void beginFunction(MIRGraph *graph, JSScript *);
> +    bool enabled();

"enabled()" is not the right name for this function. "isSpewingFunction()"?

Comment 2

[Hannes Verschore [:h4writer]]

Why hasn't this patch get into the tree? Looks very handy!

Comment 3

[Nicolas B. Pierron [:nbp]]

I kept this patch on hold because it is not thread-safe as sstangl reported.  I'll update it.

Comment 4

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9505ab7136b1

Here is the latest version of the patch, sadly, I just realized that I forgot to honor sstangl nit about adding a comment in the code to describe how to use it.

There is no threading issues since the modification of the flag is updated on the main thread before we start spewing anything.

The env variable spec:

without any IONFILTER environment variable, everything is spew as expected in debug builds.  With IONFILTER set, all spew appear except for compilations of scripts which are not referred in IONFILTER.  IONFILTER is extremelly convenient when combined with IONFLAGS=logs as it gives the ability to dump only the different compilation phase of only one function.  This starts being usefull with real-life benchmarks as well as website with a debug build of Firefox.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

https://hg.mozilla.org/mozilla-central/rev/9505ab7136b1