Closed Bug 1403438 Opened 2 years ago Closed 2 years ago

mips64-linux profiler lul

Categories

(Core :: Gecko Profiler, defect)

58 Branch
Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- 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, supporting profiler lul.


Actual results:

error "Unsupported arch"


Expected results:

supporting mips64-linux.
Attachment #8912531 - Flags: review?(n.nethercote)
Attachment #8912531 - Flags: review?(mstange)
Attachment #8912531 - Flags: review?(jseward)
Attachment #8912531 - Flags: review?(cyu)
Comment on attachment 8912531 [details] [diff] [review]
Bug-1403438-add-profiler-lul-on-mips64-linux

Cancel the request for the same reason for bug 1403438.
Attachment #8912531 - Flags: review?(cyu)
Attachment #8912531 - Flags: review?(n.nethercote)
Attachment #8912531 - Flags: review?(mstange)
Attachment #8912531 - Flags: review?(jseward)
Attachment #8913114 - Flags: review?(n.nethercote)
Attachment #8913114 - Flags: review?(mstange)
Attachment #8913114 - Flags: review?(jseward)
Comment on attachment 8913114 [details] [diff] [review]
Bug-1403438-add-profiler-lul-on-mips64-linux.patch

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

I had a quick look and it seems reasonable, but jseward is the right person to review this.

Please note that we are considering whether it is possible to remove LUL, and use MozStackWalk() and/or FramePointerStackWalk() instead, as is done on Windows and Mac. Because LUL is slow to start up and uses a lot of memory. But that won't happen for a while, if it happens at all.
Attachment #8913114 - Flags: review?(n.nethercote)
Attachment #8913114 - Flags: review?(mstange)
Comment on attachment 8913114 [details] [diff] [review]
Bug-1403438-add-profiler-lul-on-mips64-linux.patch

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

In general, it looks OK, but I have one question.  How much have you
tested it?  Does it actually work?  I am a bit surprised that the mips64
cases -- where you unwind (fp, sp, pc) -- is so similar to the x86_64
case, and that you don't need to unwind any other registers -- for example
like we have to also unwind r7, r11, r12 on arm32.

r+ with the changes below, and if this is tested and known to work.

::: tools/profiler/core/platform.cpp
@@ +106,5 @@
>  # include "EHABIStackWalk.h"
>  #endif
>  
>  // Linux builds use LUL, which uses DWARF info to unwind stacks.
> +#if defined(GP_PLAT_amd64_linux) || defined(GP_PLAT_x86_linux) || defined(GP_PLAT_mips64_linux)

Please keep the line length < 80 characters, here and everywhere else.

::: tools/profiler/lul/LulDwarf.cpp
@@ +1855,5 @@
>    */
>    return 13 * 8;
>  }
>  
> +unsigned int DwarfCFIToModule::RegisterNames::MIPS() {

Do 64- and 32-bit MIPS have exactly the same register names?
Unless they do, I would prefer you use "::MIPS64" here rather
that just "::MIPS" so as to avoid confusion.  And the same
elsewhere in the patch.

::: tools/profiler/lul/LulDwarfExt.h
@@ +1211,5 @@
>      // ARM.
>      static unsigned int ARM();
> +
> +    // MIPS.
> +    static unsigned int MIPS();

MIPS64, per comments above.

::: tools/profiler/lul/LulMain.cpp
@@ +1447,5 @@
>  ////////////////////////////////////////////////////////////////
>  
>  static const int LUL_UNIT_TEST_STACK_SIZE = 16384;
>  
> +#if defined(GP_ARCH_mips64)

I don't think you need this, and would prefer it was removed.
In the assembly sequence in GetAndCheckStackTrace, can you just
do 
   branch-and-link to the next insn
   move $tmp_reg, $31
   sd $tmp_reg, 0(%0)
?

@@ +1536,5 @@
> +    :
> +    :"r"(block)
> +    :"memory"
> +  );
> +  block[0] = __getpc();

Just set block[0] using the inline assembly, per comment above.
Attachment #8913114 - Flags: review?(jseward) → review+
Sorry, reply so late.


(In reply to Julian Seward [:jseward] from comment #5)
> Comment on attachment 8913114 [details] [diff] [review]
> Bug-1403438-add-profiler-lul-on-mips64-linux.patch
> 
> Review of attachment 8913114 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In general, it looks OK, but I have one question.  How much have you
> tested it?  Does it actually work?  I am a bit surprised that the mips64
> cases -- where you unwind (fp, sp, pc) -- is so similar to the x86_64
> case, and that you don't need to unwind any other registers -- for example
> like we have to also unwind r7, r11, r12 on arm32.

fp is mips register $30, sp is register $29.
pc usually is cp0.epc, under exception mode.




> r+ with the changes below, and if this is tested and known to work.
> 
> ::: tools/profiler/core/platform.cpp
> @@ +106,5 @@
> >  # include "EHABIStackWalk.h"
> >  #endif
> >  
> >  // Linux builds use LUL, which uses DWARF info to unwind stacks.
> > +#if defined(GP_PLAT_amd64_linux) || defined(GP_PLAT_x86_linux) || defined(GP_PLAT_mips64_linux)
> 
> Please keep the line length < 80 characters, here and everywhere else.

ok, I'll modify it.



> ::: tools/profiler/lul/LulDwarf.cpp
> @@ +1855,5 @@
> >    */
> >    return 13 * 8;
> >  }
> >  
> > +unsigned int DwarfCFIToModule::RegisterNames::MIPS() {
> 
> Do 64- and 32-bit MIPS have exactly the same register names?
> Unless they do, I would prefer you use "::MIPS64" here rather
> that just "::MIPS" so as to avoid confusion.  And the same
> elsewhere in the patch.
> 
> ::: tools/profiler/lul/LulDwarfExt.h
> @@ +1211,5 @@
> >      // ARM.
> >      static unsigned int ARM();
> > +
> > +    // MIPS.
> > +    static unsigned int MIPS();
> 
> MIPS64, per comments above.

Yes, mips32 and mips64 have same register.




> ::: tools/profiler/lul/LulMain.cpp
> @@ +1447,5 @@
> >  ////////////////////////////////////////////////////////////////
> >  
> >  static const int LUL_UNIT_TEST_STACK_SIZE = 16384;
> >  
> > +#if defined(GP_ARCH_mips64)
> 
> I don't think you need this, and would prefer it was removed.
> In the assembly sequence in GetAndCheckStackTrace, can you just
> do 
>    branch-and-link to the next insn
>    move $tmp_reg, $31
>    sd $tmp_reg, 0(%0)
> ?


Because mips platform does't have pc register, that is, you can't get pc directly.
But you can get through branch-and-link instruction indirectly. Here is using this way.


> @@ +1536,5 @@
> > +    :
> > +    :"r"(block)
> > +    :"memory"
> > +  );
> > +  block[0] = __getpc();
> 
> Just set block[0] using the inline assembly, per comment above.
(In reply to Julian Seward [:jseward] from comment #5)
> Comment on attachment 8913114 [details] [diff] [review]
> Bug-1403438-add-profiler-lul-on-mips64-linux.patch
> 
> Review of attachment 8913114 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In general, it looks OK, but I have one question.  How much have you
> tested it?  Does it actually work?  


sorry to be frankly, I just have tested the performance core function in inbound-58.0a1 and  is ok.
Or open the performance,  Menu ---> Web Developer --->  Performance, can start and stop record.
Of course, the result is ok.


Is there some test files like "tools/profiler/tests/test_asm.js" ?
(In reply to qiaopengcheng from comment #7)
> (In reply to Julian Seward [:jseward] from comment #5)
> > Comment on attachment 8913114 [details] [diff] [review]
> > Bug-1403438-add-profiler-lul-on-mips64-linux.patch
> > 
> > Review of attachment 8913114 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > In general, it looks OK, but I have one question.  How much have you
> > tested it?  Does it actually work?  
> 
> 
> sorry to be frankly, I just have tested the performance core function in
> inbound-58.0a1 and  is ok.
> Or open the performance,  Menu ---> Web Developer --->  Performance, can
> start and stop record.
> Of course, the result is ok.
> 

This is misleading, but Web Developer -> Performance *doesn't* use the native stackwalker. You need to install the gecko profiler addon from https://perf-html.io/ and use it to profile.
(In reply to Cervantes Yu [:cyu] [:cervantes] from comment #8)
> 
> This is misleading, but Web Developer -> Performance *doesn't* use the
> native stackwalker. You need to install the gecko profiler addon from
> https://perf-html.io/ and use it to profile.

But I can't open https://perf-html.io/ neither x86 nor mips64 platform.
> But I can't open https://perf-html.io/ neither x86 nor mips64 platform.

It's just a website. You should be able to load it and then click on the "install add-on" button to install the profiler add-on.
(In reply to Nicholas Nethercote [:njn] from comment #10)
> > But I can't open https://perf-html.io/ neither x86 nor mips64 platform.
> 
> It's just a website. You should be able to load it and then click on the
> "install add-on" button to install the profiler add-on.

I upload an attachment file  “open_perf-io-html_fail.png”.
That is the screenshot while opening the website.
That's strange. I just tried multiple browsers on two computers and the site worked fine for me in all cases. I don't know what the problem might be :(
(In reply to qiaopengcheng from comment #12)
> (In reply to Nicholas Nethercote [:njn] from comment #10)
> > > But I can't open https://perf-html.io/ neither x86 nor mips64 platform.
> > 
> > It's just a website. You should be able to load it and then click on the
> > "install add-on" button to install the profiler add-on.
> 
> I upload an attachment file  “open_perf-io-html_fail.png”.
> That is the screenshot while opening the website.

You can self-host the site. The source is available at https://github.com/devtools-html/perf.html if you have any networking issues.
(In reply to Cervantes Yu [:cyu] [:cervantes] from comment #14)
> (In reply to qiaopengcheng from comment #12)
> > (In reply to Nicholas Nethercote [:njn] from comment #10)
> > > > But I can't open https://perf-html.io/ neither x86 nor mips64 platform.
> > > 
> > > It's just a website. You should be able to load it and then click on the
> > > "install add-on" button to install the profiler add-on.
> > 
> > I upload an attachment file  “open_perf-io-html_fail.png”.
> > That is the screenshot while opening the website.
> 
> You can self-host the site. The source is available at
> https://github.com/devtools-html/perf.html if you have any networking issues.

Now my host-environment can't access this website for network operator.
Thank you a lot.
Keywords: checkin-needed
OS: Unspecified → Linux
Hardware: Unspecified → Other
Assignee: nobody → qiaopengcheng-hf
Attachment #8912531 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8411c490c109
Add profiler-lul on mips64-linux. r=sewardj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8411c490c109
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Last time uploading the patch is branch-57.
Amend the patch.
Attachment #8927725 - Flags: review?(n.nethercote)
Comment on attachment 8927725 [details] [diff] [review]
Bug 1403438 - Amend profile-lul code on mips64-linux

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

::: tools/profiler/core/platform.cpp
@@ +107,5 @@
>  #endif
>  
>  // Linux builds use LUL, which uses DWARF info to unwind stacks.
> +#if defined(GP_PLAT_amd64_linux) || defined(GP_PLAT_x86_linux) \
> +    || defined(GP_PLAT_mips64_linux)

Please put the `||` before the `\`. Otherwise, this looks fine.
Attachment #8927725 - Flags: review?(n.nethercote)
Attachment #8927725 - Attachment is obsolete: true
Attachment #8927757 - Flags: review?(n.nethercote)
Comment on attachment 8927757 [details] [diff] [review]
Bug 1403438 - Amend profile lul code on mips64-linux

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

::: tools/profiler/core/platform.cpp
@@ +107,5 @@
>  #endif
>  
>  // Linux builds use LUL, which uses DWARF info to unwind stacks.
> +#if defined(GP_PLAT_amd64_linux) || defined(GP_PLAT_x86_linux) ||\
> +    defined(GP_PLAT_mips64_linux)

We need space between the `||` and the `\`. Sorry I didn't make this clear.
Attachment #8927757 - Flags: review?(n.nethercote)
Attachment #8927757 - Attachment is obsolete: true
Attachment #8928038 - Flags: review?(n.nethercote)
Comment on attachment 8928038 [details] [diff] [review]
Bug 1403438 - Amend profile lul code on mips64-linux

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

Thank you!
Attachment #8928038 - Flags: review?(n.nethercote)
Attachment #8928038 - Flags: review+
Attachment #8928038 - Flags: checkin?
Keywords: checkin-needed
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
Target Milestone: mozilla58 → ---
Attachment #8928038 - Flags: checkin? → checkin+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64e81f8547bb
Amend profiler-lul code on mips64-linux. r=njn
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/64e81f8547bb
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.