Last Comment Bug 703444 - Port SPS profiler to windows
: Port SPS profiler to windows
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla11
Assigned To: :Ehsan Akhgari
:
Mentors:
Depends on: 683229 719698
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-17 16:50 PST by Benoit Girard (:BenWa)
Modified: 2012-02-01 13:59 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
A prototype of what this should look like (50.67 KB, patch)
2011-11-17 17:36 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
WIP (40.95 KB, patch)
2011-11-28 08:59 PST, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
WIP (40.72 KB, patch)
2011-11-28 19:07 PST, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
WIP (35.49 KB, patch)
2011-12-02 00:05 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
v1 (35.32 KB, patch)
2011-12-02 17:31 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
v1 (rebased bug 699918) (25.65 KB, patch)
2011-12-04 11:12 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
w/ thread join (25.90 KB, patch)
2011-12-04 15:14 PST, :Felipe Gomes (needinfo me!)
jmuizelaar: review+
ehsan: review+
Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2011-11-17 16:50:17 PST
This is analogous to bug 698002. Jeff had some ideas on how to proceed.
Comment 1 Jeff Muizelaar [:jrmuizel] 2011-11-17 17:36:53 PST
Created attachment 575346 [details] [diff] [review]
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) 2011-11-25 14:38:26 PST
Ehsan has made the following progress on this:
https://github.com/ehsan/mozilla-central/tree/spswindows
Comment 3 :Ehsan Akhgari 2011-11-28 08:59:16 PST
Created attachment 577289 [details] [diff] [review]
WIP

What I have so far.  It should build on Windows.  I haven't tested it at all yet.
Comment 4 :Ehsan Akhgari 2011-11-28 19:07:55 PST
Created attachment 577461 [details] [diff] [review]
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 me!) 2011-12-02 00:05:13 PST
Created attachment 578512 [details] [diff] [review]
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 me!) 2011-12-02 17:31:11 PST
Created attachment 578786 [details] [diff] [review]
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 me!) 2011-12-02 18:34:16 PST
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) 2011-12-03 13:31:02 PST
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) 2011-12-04 11:12:13 PST
Created attachment 578924 [details] [diff] [review]
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 me!) 2011-12-04 15:14:07 PST
Created attachment 578944 [details] [diff] [review]
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) 2011-12-04 15:35:33 PST
(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 :Ehsan Akhgari 2011-12-05 08:43:29 PST
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) 2011-12-05 08:46:23 PST
We also need the following error fixed on try:
Cannot open include file: 'stdint.h'
Comment 14 Mozilla RelEng Bot 2011-12-05 10:30:28 PST
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/b56girard@gmail.com-9e73c368c76e
Comment 15 Jeff Muizelaar [:jrmuizel] 2011-12-05 13:48:26 PST
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 me!) 2011-12-08 20:29:33 PST
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] 2011-12-10 20:44:26 PST
https://hg.mozilla.org/mozilla-central/rev/59c363453713

Note You need to log in before you can comment on or make changes to this bug.