Closed Bug 679981 Opened 13 years ago Closed 13 years ago

IonMonkey: Refactor IonSpewer to handle multiple functions.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sstangl, Assigned: sstangl)

Details

Attachments

(2 files)

The IonSpewer overwrites /tmp/ion.json at the start of function compilation, which hides output for scripts compiling multiple functions. This is marginally annoying when debugging function calls, since the CFG of the caller is always masked.

We should rewrite IonSpewer in the singleton pattern with a global object.
Additionally, the IonSpewer should have no effect (not even with respect to memory) on opt builds.
Comment on attachment 554002 [details] [diff] [review]
Refactor {Ion,C1,JSON}Spewer to handle multiple functions.

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

::: js/src/ion/C1Spewer.cpp
@@ +65,5 @@
>      spewout_ = fopen(path, "w");
>      if (!spewout_)
> +        return false;
> +
> +    return true;

return (bool)spewout_; or something like that...

::: js/src/ion/Ion.cpp
@@ +398,4 @@
>  
>      if (!BuildDominatorTree(graph))
>          return false;
> +    IonSpewPass("Dominator tree");

I used to have this pass in here because I wanted to emit the dominator tree as well. It doesn't actually change the shape of the graph; it just annotates it. Should we remove this spew pass?

::: js/src/ion/IonSpewer.cpp
@@ +45,5 @@
>  
>  using namespace js;
>  using namespace js::ion;
>  
> +// IonSpewer lingleton.

lingleton?

I hope this is a typo, and not some terrifying design pattern.

@@ +51,2 @@
>  
> +static bool LoggingChecked = false;

this doesn't appear to be used.

@@ +148,2 @@
>  {
> +    JS_ASSERT(inited_);

c1spewer should probably have a no-op endFunction()
Attachment #554002 - Flags: review?(rpearl) → review+
Thanks!

LoggingChecked is used by some other code in IonSpewer.cpp that wasn't touched.

This is a small follow-up patch that addresses the comments above and has the JSONSpewer notice when it was asked to terminate or start a new function before the previous one ended. It then correctly terminates the old function. This lets us get rid of the patch-up logic in iongraph, which didn't *really* work anyway.
Attachment #554153 - Flags: review?(rpearl)
Attachment #554153 - Flags: review?(rpearl) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/2f95cb807c67
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.