Closed Bug 703444 Opened 8 years ago Closed 8 years ago

Port SPS profiler to windows


(Core :: Graphics, defect)

Not set





(Reporter: BenWa, Assigned: ehsan)




(1 file, 6 obsolete files)

This is analogous to bug 698002. Jeff had some ideas on how to proceed.
Here's a patch that includes a mostly working mac port and the beginnings of a windows port.
Ehsan has made the following progress on this:
Attached patch WIP (obsolete) — Splinter Review
What I have so far.  It should build on Windows.  I haven't tested it at all yet.
Assignee: nobody → ehsan
Attachment #575346 - Attachment is obsolete: true
Attached patch WIP (obsolete) — Splinter Review
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!)
Attachment #577289 - Attachment is obsolete: true
No longer blocks: 683229
Depends on: 683229
Attached patch WIP (obsolete) — Splinter Review
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..
Attachment #577461 - Attachment is obsolete: true
Attached patch v1 (obsolete) — Splinter Review
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..
Attachment #578512 - Attachment is obsolete: true
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 on attachment 578786 [details] [diff] [review]

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

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

::: tools/profiler/
@@ +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/
@@ +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.
Attached patch v1 (rebased bug 699918) (obsolete) — Splinter Review
I rebased on top of bug 699918. I tested with the sample add-on and confirmed this is working.
Attached patch w/ thread joinSplinter Review
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
Attachment #578786 - Attachment is obsolete: true
Attachment #578924 - Attachment is obsolete: true
Attachment #578944 - Flags: review?
(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 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?
Attachment #578944 - Flags: review?(jmuizelaar)
Attachment #578944 - Flags: review?
Attachment #578944 - Flags: review+
We also need the following error fixed on try:
Cannot open include file: 'stdint.h'
Try run for 9e73c368c76e is complete.
Detailed breakdown of the results available here:
Results (out of 146 total builds):
    success: 123
    warnings: 17
    failure: 6
Builds available at
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.
Attachment #578944 - Flags: review?(jmuizelaar) → review+
After being bitten by 2 different compiler bugs, this has just landed on inbound. I documented the msvc8 bugs on MDN ( and as comments on the patch, to hopefully help someone trying to use intrin.h in the future.
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.