703444 - Port SPS profiler to windows

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Girard (:BenWa)]

This is analogous to bug 698002. Jeff had some ideas on how to proceed.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: A prototype of what this should look like

Here's a patch that includes a mostly working mac port and the beginnings of a windows port.

Comment 2

[Benoit Girard (:BenWa)]

Ehsan has made the following progress on this:
https://github.com/ehsan/mozilla-central/tree/spswindows

Comment 3

[(no longer active)]

Attachment: WIP

What I have so far.  It should build on Windows.  I haven't tested it at all yet.

Comment 4

[(no longer active)]

Attachment: WIP

When I said that the last patch compiles, I sort of lied.  But Felipe confirmed that this one _does_ compile (and link for that matter!)

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: WIP

The previous patch was busted due to some bitrotting and also the functions from the Thread class were missing. This patch has them all and compiles properly. I did some very basic tests, basically checked that the browser loads and the profiler starts..

Also removed the platform-macos file from the patch and Makefile since that's already on Jeff's patch..

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Attachment: v1

Cleaned up patch. I've tested that the thread is running, the tick works, GetThreadContext/Suspend/Resume works and the profile saving is producing the expected output.

I believe it's ready for review.  The changes I did from Ehsan's patch was including the Thread:: functions' bodies in platform-win32 and minor adjustments here in there due to the bitrotting. Some .h include order had to be changed to compile properly.. Also changed the way snprintf prints the %TEMP% folder. I think that was basically it..

Comment 7

[:Felipe Gomes (needinfo for replies!)]

and I also inlined mozilla::tls::create from thread_helper.h because that was causing symbol conflicts (as that header gets included from two different files)

Comment 8

[Benoit Girard (:BenWa)]

Comment on attachment 578786 [details] [diff] [review]
v1

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

Looks good, can't wait for this to land.

::: tools/profiler/Makefile.in
@@ +50,5 @@
>  
>  EXPORTS = \
>    sampler.h \
>    sps_sampler.h \
> +  thread_helper.h \

I don't see where this is used outside of tools/profiler. Do we really need to export this?

::: tools/profiler/sps/platform-win32.cc
@@ +175,5 @@
> +}
> +
> +// Close our own handle for the thread.
> +Thread::~Thread() {
> +  if (data_->thread_ != kNoThread) CloseHandle(data_->thread_);

I don't see any code to wait for the thread to exit. I'm not familiar with the windows API but I feel like it's wrong to close the handle to a running thread. We should do something similar to a Join operation. I've since fixed that in the mac backend and I believe this mistakes need to be fixed in the linux port as well. The extension will allow start/stop so this is important.

Comment 9

[Benoit Girard (:BenWa)]

Attachment: v1 (rebased bug 699918)

I rebased on top of bug 699918. I tested with the sample add-on and confirmed this is working.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Attachment: w/ thread join

Added the Thread::Join() call and tested with the profiler add-on. Added chromium license header at the top of the file.
Based on benoit's rebased version. Built and tested on top of bug 699918.

Couldn't remove thread_helper.h from the exports because nsAppRunner.cpp includes sps_sampler.h that includes thread_helper.h

Marking for review

Comment 11

[Benoit Girard (:BenWa)]

(In reply to Felipe Gomes (:felipe) from comment #10)
> Couldn't remove thread_helper.h from the exports because nsAppRunner.cpp
> includes sps_sampler.h that includes thread_helper.h

Ohh right, looks good to me.

Comment 12

[(no longer active)]

Comment on attachment 578944 [details] [diff] [review]
w/ thread join

The Win32 stuff in this patch look good.  The only change that I would suggest here is to use __declspec(selectany) instead of inline for the Windows implementation of mozilla::tls::create.

Somebody needs to review the rest of this patch.  Perhaps Jeff can do that?

Comment 13

[Benoit Girard (:BenWa)]

We also need the following error fixed on try:
Cannot open include file: 'stdint.h'

Comment 14

[Mozilla RelEng Bot]

Try run for 9e73c368c76e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9e73c368c76e
Results (out of 146 total builds):
    success: 123
    warnings: 17
    failure: 6
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-9e73c368c76e

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 578944 [details] [diff] [review]
w/ thread join

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

Things look reasonably sane.

::: tools/profiler/sps/platform.h
@@ +184,5 @@
> +    Options() : name("v8:<unknown>"), stack_size(0) {}
> +
> +    const char* name;
> +    int stack_size;
> +  };

I took this hunk out of the mac profiler. It shouldn't be needed.

@@ +230,5 @@
> +  static inline void* GetExistingThreadLocal(LocalStorageKey key) {
> +    return GetThreadLocal(key);
> +  }
> +#endif
> +

I don't think this hunk is needed either.

Comment 16

[:Felipe Gomes (needinfo for replies!)]

After being bitten by 2 different compiler bugs, this has just landed on inbound. I documented the msvc8 bugs on MDN (https://developer.mozilla.org/En/Developer_Guide/Build_Instructions/Intrin.h) and as comments on the patch, to hopefully help someone trying to use intrin.h in the future.

https://hg.mozilla.org/integration/mozilla-inbound/rev/59c363453713

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/59c363453713