974700 - cached asm.js doesn't cache profiled function info

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

If cached asm.js code is being used, Module._profiledFunctions is always empty, thus leading to no function data being sent to VTune.

I don't know if it would be prohibitive to cache the data that would be needed to build up profiledFunctions; that would be ideal so that we could recreate it.  Alternatively, we can disable caching if vtune is connected... but that has the problem of not letting us actually profile cached startup reliably.

Comment 1

[Luke Wagner [:luke]]

Oops, this isn't too hard to fix; just some extra members of the AsmJSModule that need to get serialized.

In the meantime, you can disable caching with javascript.options.parallel_parsing = false.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I don't think that's enough -- that would mean that the code would have to be cached while running under a profiler for that information to get serialized, since it's not saved normally.

My current source hack is to just disable cache loading if vtune profiling is enabled, which gets me around the problem, but would be nice to have a full fix for this so that people can use this without too much worry.

Comment 3

[Luke Wagner [:luke]]

We can just take out the IsVTuneProfilingActive() check in GenerateCode() and then we'll always record the ProfiledFunction information when --enable-profiling.

Comment 4

[Luke Wagner [:luke]]

Attachment: fix-vtune

Does this patch fix it?  Note: there might be a few trivial compile errors on Windows+VTune; I only tested with --enable-profiling on Linux+perf (which mostly reuses the same code).

Comment 5

[Douglas Crosher [:dougc]]

Might it be useful to add a flag to the data stored in the cache to note when the profiling info is included in the cache.  This could allow the recording of the profiling info to be avoided when not needed.  Might the memory usage be significant for some development work - the profiling build is my default.  Might this be needed in the case that a non-profiling build firstly compiles the asm.js code and stores it in the cache, and then the user rebuilds with profiling enabled?

Comment 6

[Luke Wagner [:luke]]

I'm just storing per-function profiling info which should be relatively small.  I just realized the patch only always stores profiling info for VTune, not Linux perf.  I can fix that.

> Might this be needed in the case that a non-profiling build
> firstly compiles the asm.js code and stores it in the cache, and then the
> user rebuilds with profiling enabled?

By "profiling build" do you mean FF itself (--enable-profiling) or do you mean FF (with --enable-profiling) compiled when profiling is active (IsVTuneProfilingActive()/PerfFuncEnabled())?  In the former case, an --enable-profiling build won't get cache hits from a --disable-profiling build (the buildid of FF is part of the cache key).  In the latter case, then profiling data is stored even when profiling isn't active, so it'll already be present when profiling is enabled.

Comment 7

[Luke Wagner [:luke]]

Attachment: fix-vtune

Oops, I meant to flag Vlad for review in the last patch.  This patch has a few tweaks.

Comment 8

[Luke Wagner [:luke]]

And by review I meant "feedback" to see if this actually fixes the problem.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8379755 [details] [diff] [review]
fix-vtune

I admit I haven't had a chance to test it yet, but it looks like it will get the job done.

Comment 10

[Luke Wagner [:luke]]

Could I leave it f?you until you do have a chance to try whether it actually works?

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ah I guess you asked if you can leave it f? but didn't actually leave it f? :)

So, I lied earlier.  This did not work for me after a reload/rerun; functions showed up as addresses that were previously named.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I lied in comment #11.  I was looking at a different view of the profile, and I thought I was seeing addresses where I should have seen functions, but they were actual valid addresses (inside some kernel and gfx drivers).  Final verdict: patch works fine.

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 8379755 [details] [diff] [review]
fix-vtune

woot, thanks

Comment 14

[Luke Wagner [:luke]]

Attachment: fix-vtune

Oops, previous patch built with --enable-vtune but not --enable-perf.

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8396524 [details] [diff] [review]
fix-vtune

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

I was trying to find a single constructive comment on the new code, but can't find any. Great work!

::: js/src/jit/AsmJSModule.h
@@ -448,5 @@
>          for (unsigned i = 0; i < profiledFunctions_.length(); i++)
>              profiledFunctions_[i].trace(trc);
>  #endif
>  #if defined(JS_ION_PERF)
> -        for (unsigned i = 0; i < profiledFunctions_.length(); i++)

wow, that pretty looks like a bug, whenever perf and vtune are activated in the mean time. Thanks for deleting that!

Comment 16

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb79c7e18cd

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4bb79c7e18cd