Closed Bug 1450185 Opened 2 years ago Closed 2 years ago

Implement lul profiler for aarch64

Categories

(Core :: Gecko Profiler, enhancement)

ARM64
Unspecified
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

bug 1360322 turns on gecko profiler for aarch64, but lul doesn't.
Blocks: Fennec-ARM64
Assignee: nobody → m_kato
Blocks: 1435934
Attachment #8967642 - Flags: review?(mstange) → review?(jseward)
Blocks: 1350500
Julian, ping.  This issue depends on symbol resolution support on Android's gecko profiler (bug 1350500).  To land bug 1350500, this fix is required.
Flags: needinfo?(jseward)
Comment on attachment 8967642 [details]
Bug 1450185 - Implement DWARF stack walker for aarch64.

https://reviewboard.mozilla.org/r/236372/#review244544

Thank you for doing this!  Basically it looks good.
r+ if you fix the 3 comments below.

::: commit-message-da809:9
(Diff revision 1)
> +not first frame.
> +
> +I test by gtest on Linux/aarch64, and profiler works on Android/aarch64.
> +
> +EM_AARCH64 might not be defined on our builders since headers are old, so
> +this define is needed. And mAdminThreadId is unused on release build and

Regarding mAdminThreadId, I see what the problem is (yes, it's unused on release builds).  But I don't feel happy about the "Unused << mAdminThreadId" fix.

Please remove that change.  Clearly this still needs to be fixed, otherwise the warnings will break your build (I assume).  Given that (1) it's unlikely that people will use the profiler in debug builds, (2) failure of these assertions is likely to lead to deadlocking or crashing in the profiler, and (3) they don't occur on high-frequency paths, I propose that a better fix is to convert them to MOZ_RELEASE_ASSERTs.  We should do that in a separate bug.

::: tools/profiler/core/PlatformMacros.h:71
(Diff revision 1)
>  #elif defined(__linux__) && defined(__arm__)
>  # define GP_PLAT_arm_linux 1
>  # define GP_ARCH_arm 1
>  # define GP_OS_linux 1
>  
> +#elif defined(__linux__) && defined(__aarch64__)

I would prefer to use GP_PLAT_arm64_{linux,android} and GP_ARCH_arm64 instead of the _aarch64_ naming, so as to be more consistent with the existing 32-bit arm naming.  Please rename these 3 preprocessor symbols accordingly.

::: tools/profiler/lul/LulDwarfSummariser.cpp:313
(Diff revision 1)
> +  if (mCurrRules.mSPexpr.mHow == UNKNOWN) {
> +    mCurrRules.mSPexpr = LExpr(NODEREF, DW_REG_CFA, 0);
> +  }
>  #elif defined(GP_ARCH_amd64) || defined(GP_ARCH_x86)
>  
>    // ---------------- x64/x86 ---------------- //

Nit: please put an equivalent spacer comment like this at the start of the arm64 summarisation cases.
Attachment #8967642 - Flags: review?(jseward) → review+
(In reply to Julian Seward [:jseward] from comment #3)
> Comment on attachment 8967642 [details]
> Bug 1450185 - Implement DWARF stack walker for aarch64.
> 
> https://reviewboard.mozilla.org/r/236372/#review244544
> 
> Thank you for doing this!  Basically it looks good.
> r+ if you fix the 3 comments below.
> 
> ::: commit-message-da809:9
> (Diff revision 1)
> > +not first frame.
> > +
> > +I test by gtest on Linux/aarch64, and profiler works on Android/aarch64.
> > +
> > +EM_AARCH64 might not be defined on our builders since headers are old, so
> > +this define is needed. And mAdminThreadId is unused on release build and
> 
> Regarding mAdminThreadId, I see what the problem is (yes, it's unused on
> release builds).  But I don't feel happy about the "Unused <<
> mAdminThreadId" fix.
> 
> Please remove that change.  Clearly this still needs to be fixed, otherwise
> the warnings will break your build (I assume).  Given that (1) it's unlikely
> that people will use the profiler in debug builds, (2) failure of these
> assertions is likely to lead to deadlocking or crashing in the profiler, and
> (3) they don't occur on high-frequency paths, I propose that a better fix is
> to convert them to MOZ_RELEASE_ASSERTs.  We should do that in a separate bug.

OK, I will remove Unused << mAdminThreadId.

But You know, this fix is for the following warning.  So, it causes build error on android/aarch64 due to warnings-as-error on task cluster.

 4:30.85 warning: tools/profiler/lul/LulMain.h:374:7 [-Wunused-private-field] private field 'mAdminThreadId' is not used

Lul constructor is only used by https://searchfox.org/mozilla-central/rev/8f06c1b9a080b84435a2906e420fe102e1ed780b/tools/profiler/core/platform-linux-android.cpp#410 and gtests, so it will use on android/aarch64 only.

To resolve this unused warning, I will convert it to MOZ_RELEASE_ASSERT to land this if I cannot use Unused << foo.
Depends on: 1456382
> (3) they don't occur on high-frequency paths, I propose that a better fix is
> to convert them to MOZ_RELEASE_ASSERTs.  We should do that in a separate bug.

I filed as bug 1456382
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1893ddb56c3b
Implement DWARF stack walker for aarch64. r=jseward
https://hg.mozilla.org/mozilla-central/rev/1893ddb56c3b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Flags: needinfo?(jseward)
You need to log in before you can comment on or make changes to this bug.