Closed Bug 881882 Opened 7 years ago Closed 7 years ago

OdinMonkey: report compile time and slow functions in asm.js success message

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
Until bug 875174, it'll be useful to report which functions are taking forever to compile so that they can be out-of-lined from the asm.js module (and thus avoid mandatory Ion compilation).  This patch includes this information at the end of the "Successfully compiled asm.js" console message.

Note: parallel compilation makes all functions take longer and the timing has more variance.  Toggle javascript.options.ion.parallel_compilation to get a more accurate measurement.
Attachment #761131 - Flags: review?(sstangl)
erm, pretty sure that won't work on Windows... use PR_Now() or whatever the JS equivalent is?
JS_Now (in jsapi.h) uses PRMJ_Now and I see a (rather long and involved) implementation on Windows.
Yeah, I think on windows it uses QueryPerformanceCounter and does the right goop to get the highest precision possible.  You could just use QPC directly on windows for timing; it'll be incorrect on some platforms, but we don't really care about those.  QPC should be quite fast to call directly.
... or I could just keep PRMJ_Now and avoid platform-specific #ifdefs :)  The performance and precision of these calls isn't particularly important.
Oh, yeah, that sounds good.  I thought you were trying to get to really low-impact, but PRMJ_Now should be fine on Windows as well in normal usage.
Attached patch rm JS_ASMJSSplinter Review
This is a minor cleanup (since now JS_ION iff JS_ASMJS) that came up while writing the main patch.
Attachment #761213 - Flags: review?(sstangl)
I'm assuming that the patch from Comment 0 needs refreshing with use of PRMJ_Now() and friends before review.
Oh, HAH, I see what Vlad was going on about, the patch in comment 0 is totally my weeks-old ad hoc measurement patch!  (I was like, "comeon dude, PRMJ_Now will totally work on windows, that's the point".)  The REAL patch is totally different (and rather more involved).
Attached patch real patchSplinter Review
Attachment #761131 - Attachment is obsolete: true
Attachment #761131 - Flags: review?(sstangl)
Attachment #761573 - Flags: review?(sstangl)
Comment on attachment 761573 [details] [diff] [review]
real patch

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

::: js/src/ion/AsmJS.cpp
@@ +1256,5 @@
>      }
>  
> +    static const unsigned SLOW_FUNCTION_THRESHOLD_MS = 250;
> +
> +    bool reportCompileTime(ParseNode *fn, unsigned ms) {

maybeReportCompileTime()?

@@ +1483,5 @@
>  
> +    void buildCompilationTimeReport(ScopedJSFreePtr<char> *out) {
> +        ScopedJSFreePtr<char> slowFuns;
> +        if (!slowFunctions_.empty()) {
> +            slowFuns.reset(JS_smprintf("; %d slow functions: ", slowFunctions_.length()));

"slow functions" could mean that the functions execute slowly. Maybe "%d functions compile slowly:"?

@@ +1491,5 @@
> +                SlowFunction &func = slowFunctions_[i];
> +                JSAutoByteString name;
> +                if (!js_AtomToPrintableString(cx_, func.name, &name))
> +                    return;
> +                slowFuns.reset(JS_smprintf("%s%s:%u:%u (%ums)%s", slowFuns.get(),

This is pretty gross. Is there some reason why we can't just allocate a large buffer and snprintf into it?

@@ +1492,5 @@
> +                JSAutoByteString name;
> +                if (!js_AtomToPrintableString(cx_, func.name, &name))
> +                    return;
> +                slowFuns.reset(JS_smprintf("%s%s:%u:%u (%ums)%s", slowFuns.get(),
> +                                           name.ptr(), func.line, func.column, func.ms,

It may also be interesting to show each function's compilation time as % of total compilation time.
Attachment #761573 - Flags: review?(sstangl) → review+
Attachment #761213 - Flags: review?(sstangl) → review+
https://hg.mozilla.org/mozilla-central/rev/00a8c9c3fe5d
https://hg.mozilla.org/mozilla-central/rev/f8f6c2b1a92e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Actually, the percentages are rather misleading in the default (parallel compilation) mode since they can add up to more than 100%.  There also isn't a simple way to fix it (summing up the totals of all the slow functions only tells you the % of slow time, what we'd need is to sum up total parallel compilation time).  For now, just removing:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5527906f11f5
Resolution: FIXED → WONTFIX
Well, overall the bug is fixed (times are reported).  The only thing removed in comment 13 was the percentages.
Resolution: WONTFIX → FIXED
Depends on: 885103
Is there a reason JS_ASMJS couldn't be made optional again, or a compile-time option? Now, to get BaselineCompiler working, I have to implement ASMJS also (even though I'm not using it), and I didn't have to before.

I would gladly submit a patch for this, but I want to know if I should bother first. Right now I have it disabled in my PPC build while I try to get Baseline off the ground.
(This is the wrong place for that question [js groups better], but please no -- fewer compile flags the better.  I understand it's more burden when porting, but it makes life simpler and more consistent in the long run.)
So much for ease of portability.
There is another bug (can't find it) where we talked about having separate JS_BASELINE_JIT and JS_ION flags, which is, it sounds like, what you really want.  Several people want this so I think we'd be happy to have those patches (in a separate bug, though).
Yes, that would be fine (I'm pretty much implementing that anyway). I can spin it into another bug, but I can't find anything else referencing a JS_BASELINE_JIT flag. I'll open a new one if no one remembers where it is.
Yeah, I don't remember where it is, new bug sounds right.  cc me and :jandem
You need to log in before you can comment on or make changes to this bug.