Closed Bug 1403421 Opened 4 years ago Closed 4 years ago

mips64 performance tools profiler

Categories

(Core :: Gecko Profiler, defect)

58 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: qiaopengcheng-hf, Assigned: qiaopengcheng-hf)

Details

Attachments

(3 files, 3 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux mips64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170925180245

Steps to reproduce:

on mips64-linux platform,
./mach configure
./mach build


Actual results:

error "Unsupported platform"


Expected results:

can build sucessfully.
Attachment #8912498 - Flags: review?(n.nethercote)
Attachment #8912498 - Flags: review?(mstange)
Attachment #8912498 - Flags: review?(jseward)
Attachment #8912498 - Flags: review?(cyu)
Comment on attachment 8912498 [details] [diff] [review]
Bug-1403421-supporting-mips64-linux-performance, tools-profiler

Thanks for the patch, but I would cancel the request since I am not the right person to review mips64 support of the gecko profiler.

Also please include 8 lines of context in the patch. See https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I...
Attachment #8912498 - Flags: review?(cyu)
Comment on attachment 8912498 [details] [diff] [review]
Bug-1403421-supporting-mips64-linux-performance, tools-profiler

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

I don't know anything about MIPS, but this all looks reasonable. I assume you have actually run the profiler on MIPS64/Linux and confirmed that it works?

You haven't defined HAVE_NATIVE_UNWIND, so you will get pseudostacks but no native stack traces.

MIPS qualifies as a tier 3 platform (see https://developer.mozilla.org/en-US/docs/Supported_build_configurations for an explanation) so it's possible that this code will get broken and you'll be responsible for maintaining it. Having said that, this patch is little enough code that it's unlikely to get broken.

Can I ask why you are interested in running the profiler on MIPS/Linux?

::: tools/profiler/core/PlatformMacros.h
@@ +62,5 @@
>  
> +#elif defined(__linux__) && defined(__mips64)
> +# define GP_PLAT_mips64_linux 1
> +# define GP_ARCH_mips64 1
> +# define GP_OS_linux 1

Please add GP_PLAT_mips64_linux and define GP_ARCH_mips64 to the list of #undef'd constants at the top of this file.

::: tools/profiler/core/platform-linux-android.cpp
@@ +104,5 @@
>    aRegs.mLR = reinterpret_cast<Address>(mcontext.regs[30]);
> +#elif defined(GP_ARCH_mips64)
> +  aRegs.mPC = reinterpret_cast<Address>(mcontext.pc);
> +  aRegs.mSP = reinterpret_cast<Address>(mcontext.gregs[29]);
> +  aRegs.mFP = reinterpret_cast<Address>(mcontext.gregs[30]);

Are the MIPS registers really the same names as the ARM ones? Interesting.
Attachment #8912498 - Flags: review?(n.nethercote)
Attachment #8912498 - Flags: review?(mstange)
Attachment #8912498 - Flags: review?(jseward)
Attachment #8912498 - Flags: feedback+
(In reply to Nicholas Nethercote [:njn] from comment #3)
> Comment on attachment 8912498 [details] [diff] [review]
> Bug-1403421-supporting-mips64-linux-performance, tools-profiler
> 
> Review of attachment 8912498 [details] [diff] [review]:
> -----------------------------------------------------------------

> 
> ::: tools/profiler/core/platform-linux-android.cpp
> @@ +104,5 @@
> >    aRegs.mLR = reinterpret_cast<Address>(mcontext.regs[30]);
> > +#elif defined(GP_ARCH_mips64)
> > +  aRegs.mPC = reinterpret_cast<Address>(mcontext.pc);
> > +  aRegs.mSP = reinterpret_cast<Address>(mcontext.gregs[29]);
> > +  aRegs.mFP = reinterpret_cast<Address>(mcontext.gregs[30]);
> 
> Are the MIPS registers really the same names as the ARM ones? Interesting.



This is right. 
ARM's is "mcontext.regs"  while MIPS is  "mcontext.gregs".
You can identify it in mips-linux OS ,usually path "/usr/include/sys/ucontext.h".

My PC is mips64 Fedora21.
qiao:~/work_qiao/linux-3.10$ uname -a
Linux localhost.localdomain 3.10.84-16.fc21.loongson.mips64el #1 SMP PREEMPT Fri Jul 21 15:36:21 CST 2017 mips64 mips64 mips64 GNU/Linux

firefox is
57.0a1 (2017-09-27) (64-bit)

and the menu ---> Web Developer ---> Performance, be used normally under safemode.

Also working in firefox52 and firefox45 sucessfully, while the codes of firefox52 and firefox45 are only in our opensource git respository.


Why we are interested in running the profiler on MIPS/Linux?
Because we are working in a Chinese CPU designing company while CPU arch is mips64.
We have to porting the opensource softwares related mips64.


We also realized that mips64-linux desktop-PC is popular, like our mozilla community.
Attached file ucontext.h
Hi Nicholas.

the attach file "ucontext.h" has been uploaded.
(In reply to Nicholas Nethercote [:njn] from comment #3)
> Comment on attachment 8912498 [details] [diff] [review]
> Bug-1403421-supporting-mips64-linux-performance, tools-profiler
> 
> Review of attachment 8912498 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't know anything about MIPS, but this all looks reasonable. I assume
> you have actually run the profiler on MIPS64/Linux and confirmed that it
> works?
> 
> You haven't defined HAVE_NATIVE_UNWIND, so you will get pseudostacks but no
> native stack traces.
> 
> MIPS qualifies as a tier 3 platform (see
> https://developer.mozilla.org/en-US/docs/Supported_build_configurations for
> an explanation) so it's possible that this code will get broken and you'll
> be responsible for maintaining it. Having said that, this patch is little
> enough code that it's unlikely to get broken.
> 
>

HAVE_NATIVE_UNWIND
 Bug-1403438-add-profiler-lul-on-mips64-linux for https://bugzilla.mozilla.org/show_bug.cgi?id=1403438
Attachment #8913112 - Flags: review?(n.nethercote)
Attachment #8913112 - Flags: review?(mstange)
Attachment #8913112 - Flags: review?(jseward)
Comment on attachment 8913112 [details] [diff] [review]
Bug-1403421-supporting-mips64-linux-performance.patch

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

As mentioned elsewhere, you only need to ask one person to review each change.

Just one small thing to fix, see below. Thanks.

::: tools/profiler/core/PlatformMacros.h
@@ +20,5 @@
>  #undef GP_PLAT_amd64_linux
>  #undef GP_PLAT_amd64_darwin
>  #undef GP_PLAT_x86_windows
>  #undef GP_PLAT_amd64_windows
> +#undef GP_PLAT_mips64_linux

These are grouped by OS, so please move the new entry just below GP_PLAT_amd64_linux.
Attachment #8913112 - Flags: review?(n.nethercote)
Attachment #8913112 - Flags: review?(mstange)
Attachment #8913112 - Flags: review?(jseward)
Attachment #8913112 - Flags: feedback+
Attachment #8912498 - Attachment is obsolete: true
Attachment #8913112 - Attachment is obsolete: true
Attachment #8913493 - Flags: review?(n.nethercote)
Attachment #8913493 - Attachment is obsolete: true
Attachment #8913493 - Flags: review?(n.nethercote)
Attachment #8913495 - Flags: review?(n.nethercote)
Comment on attachment 8913495 [details] [diff] [review]
Bug-1403421-supporting-mips64-linux-performance.patch

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

Looks good! Thank you for the changes.

Have you done a try push to make sure it builds on all Tier-1 platforms? I'm not sure if you are familiar with try pushes; if not, https://wiki.mozilla.org/ReleaseEngineering/TryServer has the details. A run with the configuration of "try: -b do -p all -u none -t none" will be fine for that patch.

If you don't have Level 1 access required to do a try push, I can do it for you, but I won't get to it until Monday next week.
Attachment #8913495 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #12)
> Comment on attachment 8913495 [details] [diff] [review]
> Bug-1403421-supporting-mips64-linux-performance.patch
> 
> Review of attachment 8913495 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! Thank you for the changes.
> 
> Have you done a try push to make sure it builds on all Tier-1 platforms? I'm
> not sure if you are familiar with try pushes; if not,
> https://wiki.mozilla.org/ReleaseEngineering/TryServer has the details. A run
> with the configuration of "try: -b do -p all -u none -t none" will be fine
> for that patch.
> 
> If you don't have Level 1 access required to do a try push, I can do it for
> you, but I won't get to it until Monday next week.


Hi,Nicholas!
Should I reupload the patch ?
I pulled down the inbound code yesterday, and tested the performance is OK after adding the patch.
If you have an updated version, please attach it to this bug.
Attached image inbound_58.0a1.png
(In reply to Nicholas Nethercote [:njn] from comment #14)
> If you have an updated version, please attach it to this bug.

This is the newest patch.
I have tested on firefox-Nightly-58.0a1 (2017-11-03) (64-bit) with this patch.
Keywords: checkin-needed
Assignee: nobody → qiaopengcheng-hf
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bbac53ac792
Support mips64-linux performance. r=njn
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9bbac53ac792
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.