Closed
Bug 703444
Opened 13 years ago
Closed 13 years ago
Port SPS profiler to windows
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: BenWa, Assigned: ehsan.akhgari)
References
Details
Attachments
(1 file, 6 obsolete files)
25.90 KB,
patch
|
jrmuizel
:
review+
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
This is analogous to bug 698002. Jeff had some ideas on how to proceed.
Comment 1•13 years ago
|
||
Here's a patch that includes a mostly working mac port and the beginnings of a windows port.
Reporter | ||
Comment 2•13 years ago
|
||
Ehsan has made the following progress on this: https://github.com/ehsan/mozilla-central/tree/spswindows
Assignee | ||
Comment 3•13 years ago
|
||
What I have so far. It should build on Windows. I haven't tested it at all yet.
Assignee | ||
Comment 4•13 years ago
|
||
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•13 years ago
|
Comment 5•13 years ago
|
||
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
Comment 6•13 years ago
|
||
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
Comment 7•13 years ago
|
||
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•13 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•13 years ago
|
||
I rebased on top of bug 699918. I tested with the sample add-on and confirmed this is working.
Comment 10•13 years ago
|
||
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•13 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•13 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•13 years ago
|
||
We also need the following error fixed on try: Cannot open include file: 'stdint.h'
Comment 14•13 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 15•13 years ago
|
||
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+
Comment 16•13 years ago
|
||
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•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/59c363453713
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in
before you can comment on or make changes to this bug.
Description
•