The default bug view has changed. See this FAQ.

Port SPS profiler to windows

RESOLVED FIXED in mozilla11

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: BenWa, Assigned: Ehsan)

Tracking

unspecified
mozilla11
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

5 years ago
This is analogous to bug 698002. Jeff had some ideas on how to proceed.
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.
(Reporter)

Comment 2

5 years ago
Ehsan has made the following progress on this:
https://github.com/ehsan/mozilla-central/tree/spswindows
(Assignee)

Comment 3

5 years ago
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.
Assignee: nobody → ehsan
Attachment #575346 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 4

5 years ago
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!)
Attachment #577289 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
No longer blocks: 683229
Depends on: 683229
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..
Attachment #577461 - Attachment is obsolete: true
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..
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)
(Reporter)

Comment 8

5 years ago
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.
(Reporter)

Comment 9

5 years ago
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.
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
Attachment #578786 - Attachment is obsolete: true
Attachment #578924 - Attachment is obsolete: true
Attachment #578944 - Flags: review?
(Reporter)

Comment 11

5 years ago
(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.
(Assignee)

Comment 12

5 years ago
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+
(Reporter)

Comment 13

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

Comment 14

5 years ago
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 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 (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
https://hg.mozilla.org/mozilla-central/rev/59c363453713
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Depends on: 719698
You need to log in before you can comment on or make changes to this bug.