1403438 - mips64-linux profiler lul

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[qiaopengcheng]

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.

Comment 1

[qiaopengcheng]

Attachment: Bug-1403438-add-profiler-lul-on-mips64-linux

Comment 2

[Cervantes Yu [:cyu] [:cervantes]]

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.

Comment 3

[qiaopengcheng]

Attachment: Bug-1403438-add-profiler-lul-on-mips64-linux.patch

Comment 4

[Nicholas Nethercote [inactive]]

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.

Comment 5

[Julian Seward [:jseward]]

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.

Comment 6

[qiaopengcheng]

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.

Comment 7

[qiaopengcheng]

(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" ?

Comment 8

[Cervantes Yu [:cyu] [:cervantes]]

(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.

Comment 9

[qiaopengcheng]

(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.

Comment 10

[Nicholas Nethercote [inactive]]

> 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.

Comment 11

[qiaopengcheng]

Attachment: open_perf-io-html_fail.png

Comment 12

[qiaopengcheng]

(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.

Comment 13

[Nicholas Nethercote [inactive]]

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 :(

Comment 14

[Cervantes Yu [:cyu] [:cervantes]]

(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.

Comment 15

[qiaopengcheng]

(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.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8411c490c109
Add profiler-lul on mips64-linux. r=sewardj

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8411c490c109

Comment 18

[qiaopengcheng]

Attachment: Bug 1403438 - Amend profile-lul code on mips64-linux

Last time uploading the patch is branch-57.
Amend the patch.

Comment 19

[Nicholas Nethercote [inactive]]

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.

Comment 20

[qiaopengcheng]

Attachment: Bug 1403438 - Amend profile lul code on mips64-linux

Comment 21

[Nicholas Nethercote [inactive]]

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.

Comment 22

[qiaopengcheng]

Attachment: Bug 1403438 - Amend profile lul code on mips64-linux

Comment 23

[Nicholas Nethercote [inactive]]

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!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64e81f8547bb
Amend profiler-lul code on mips64-linux. r=njn

Comment 25

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/64e81f8547bb