PseudoContext::sampleContext() is always called immediately after
profiler_get_pseudo_stack(). This patch introduces profiler_set_js_context()
and profiler_clear_js_context(), which replace the profiler_get_pseudo_stack()
+ sampleContext() pairs. This takes us a step closer to not having to export
PseudoStack outside the profiler.
--HG--
extra : rebase_source : 8558d1600eafd395cc696d31f3d21fb52a1a74b0
This is more of a workaround than a real fix, but the old code wasn't working
for non-main threads either, and we'd like to change the way this information
is computed anyway (bug 1340714) and then we won't need CheckResponsivenessTask
any more.
MozReview-Commit-ID: FGiomjwpk3z
--HG--
extra : rebase_source : 95fc57c0622fdbaa928700a971a4ce839ad92797
Bug 1341255 changed PseudoStack::mStack to a js::ProfileEntry[], so we don't
need this assertion any more.
--HG--
extra : rebase_source : 7fcb5218db56b5a9409d81ff67d653e9db37414e
The most important of these is the one in profiler_get_profile_jsobject().
Currently the JS_ParseJSON() call frequently leads to deadlock, due to
profiler_log() being called within it.
--HG--
extra : rebase_source : 7825229da61ad0d567c11bd455a7a9b8f25b5b78
This patch does the following.
- Introduces NotifyObservers() for the simple notification cases in
platform.cpp.
- Removes profiler_lock() and profiler_unlock() because they do notifications
that the profiler add-on no longer listens for.
--HG--
extra : rebase_source : 77a1868ba494dea314702bbdf9478a1da36c9efb
Calling NotifyObserver() with gPSMutex locked is a bad idea; arbitrary code can
run in observers, which can easily include code that calls other profiler
functions that themselves lock gPSMutex, causing deadlock. This has been seen
in practise with locked_profiler_stop().
This patch moves all but one of the NotifyObserver() calls in platform.cpp to
after the sections where gPSMutex is locked. The remaining call (for the
"profiler-subprocess") is harmless, because it just calls a simple callback
implemented within platform.cpp, and hard to move.
In the future we plan to allow profiler_start() and profiler_stop() to be
called from different threads. When that happens, it will be possible for the
"profiler-start" and "profiler-stop" notifications to arrive out of order.
Unfortunately this seems difficult to avoid. (Well, recursive mutexes would
make this problem much easier, but we don't have those...)
--HG--
extra : rebase_source : 78455c4b2d93a0d4110cdd401d6b542b641dd217
SamplerThread::Join() is always called just before the SamplerThread object is
deleted. Might as well combine them.
--HG--
extra : rebase_source : d7fc013b83ec77e347ffc3929533c5259df77e06
profiler_start() can call locked_profiler_stop(). In that case it must then
call SamplerThread::Join() in order to free the SamplerThread, but it currently
doesn't.
This patch adds such a call. It also marks locked_profiler_start() with
MOZ_MUST_USE to make this mistake less likely in the future.
--HG--
extra : rebase_source : 9045561aa54b54099e710c3eaea5ac533ca5485b
This patch properly synchronizes all the global state in platform*.cpp, which
gets us a long way towards implementing bug 1330184.
- Most of the global state goes in a new class, ProfilerState, with a single
instance, gPS. All accesses to gPS are protected by gPSMutex. All functions
that access ProfilerState require a token proving that gPS is locked; this
makes things much clearer.
gRegisteredThreadsMutex is removed because it is subsumed by gPSMutex.
- gVerbosity, however, does not go in ProfilerState. It stays separate, and
gains its own mutex, gVerbosityMutex.
Also, the tracking of the current profiler state is streamlined. Previously it
was tracked via:
- stack_key_initialized, gInitCount, gSampler, gIsProfiling, gIsActive, and
gIsPaused.
Now it is tracked via:
- gPS, gPS->sActivity, and gPS->mIsPaused.
This means that the Sampler class is no longer necessary, and the patch removes
it.
Other changes of note made by the patch are as follows.
- It removes ThreadInfo::{mMutex,GetMutex}. This mutex was only used in two
places, and both these are now protected by gPSMutex.
- It tweaks the LOG calls. All the main functions (init(), shutdown(), start(),
stop()) now do consistent BEGIN/END logging, and a couple of other low-value
incidental LOG calls have been removed.
- It adds a lot of release assertions requiring that gPS be initialized (e.g.
profiler_init() has been called but profiler_shutdown() has not).
- It uses alphabetical order for everything involving profiler feature names.
- It removes Platform{Start,Stop}() and SamplerThread::{Start,Stop}Sampler().
These are no longer necessary now that SamplerThread::sInstance has been
replaced with ProfilerState::mSamplerThread which allows more direct access
to the current SamplerThread instance.
- It removes PseudoStack::mPrivacyMode. This was derived from the "privacy"
feature, and we now use gPS->mFeaturePrivacy directly, which is simpler.
It also replaces profiler_in_privacy_mode() with
profiler_is_active_and_not_in_privacy_mode(), which avoids an unnecessary
lock/unlock of gPSMutex on a moderately hot path.
Finally, the new code does more locking than the old one. A number of operation
The following operations now lock a mutex when they previously didn't; the
following are ones that are significant, according to some ad hoc profiling.
- profiler_tracing()
- profiler_is_active()
- profiler_is_active_and_not_in_privacy_mode()
- profiler_add_marker()
- profiler_feature_active()
- SamplerThread::Run() [when the profiler is paused]
All up this roughly doubles the amount of mutex locking done by the profiler.
It's probably possible to avoid this increase by allowing careful unlocked
access to three of the fields in ProfilerState (mActivityGeneration,
mFeaturePrivacy, mStartTime), but this should only be done as a follow-up if
the extra locking is found to be a problem.
--HG--
extra : rebase_source : c2e41231f131b3e9ccd23ddf43626b54ccc77b7b
Instead of nulling ThreadInfo::mPseudoStack, the patch changes things so that
ownership of the PseudoStack is transferred to the ThreadInfo. This avoids
crashes in some cases.
The patch also makes ThreadInfo::mPseudoStack a NotNull<>.
--HG--
extra : rebase_source : 95ace8886092ebe17ac0f4431c8c0936946c1f44
Because profiler_time() is going to need the global lock when I add it, and the
lock will already be held when streaming is happening, so it'll cause the
thread to deadlock itself.
Unfortunately this requires adding an |aStartTime| parameter to a lot of
functions, but this is the least worst way I can think of handling it.
This also removes the need for one of the profiler_time() functions, which the
patch removes.
This makes it more like platform-{macos,win32}.cpp, and will make it easier to
introduce locking around all the globals in platform.cpp.
The following things were moved into the new class.
- Types:
- SigHandlerCoordinator
- Functions:
- SigProfHandler()
- SigprofSender() as Run()
- Variables:
- gOldSigprofHandler as mOldSigprofHandler
- gSigprofSenderThread as mThread
- gIntervalMicro as mIntervalMicro
- gSigHandlerCoordinator as sSigHandlerCoordinator
sInstance is the singleton instance. PlatformStart() and PlatformStop() now
just delegate to StartSampler/StopSampler.
The patch also does the following tidy-ups.
Linux:
- gWasPaused is now cleared in the parent and child processes after forking.
- LUL: inlined and removed gLUL_initialization_routine().
- LUL: now only calling EnabledUnwinding() and doing the unit tests on the
first call to profiler_start(), instead of all of them.
Mac:
- Removed a useless call to pthread_self() -- mThread is already set by the
earlier call to pthread_create().
- Removed some low-value checking involving kNoThread.
Mac + Win:
- Renamed SamplerThread::mInstance as sInstance, because it's static.
- Merged SamplerThread::Start() with the constructor.
- Tweaked how mInterval/mIntervalMicro is initialized, so it can become const.
All platforms:
- Converted NULL to nullptr throughout.
- A few other very minor clean-ups, e.g. formatting.
--HG--
extra : rebase_source : 4d10be873c4a7544f450e6a3b3d86c5d5b2fb603
When ProfilerBuffer::reset() is called, DuplicateLastSample() will start
failing for all sleeping threads because there will be no prior thread data in
the buffer to duplicate. But the sampling loop doesn't detect such failure.
This causes two problems:
- Missing samples.
- CPU usage goes through the roof, because each time around the sampling loop
the length of the failing search increases.
The fix is simple: detect failure in the sampling loop and do a normal sample
in that case.
The patch also removes ThreadInfo::DuplicateLastSample(), because it just calls
onto ProfileBuffer::DuplicateLastSample().
--HG--
extra : rebase_source : d51709994e701fdd63c292df5f723a2d43c4d754
It's now a very thin wrapper around ThreadInfo, and so can be removed.
The patch also has the bonus of setting mIsMainThread correctly for the
ThreadInfos that used to be SyncProfiles (i.e. the ones created in
profiler_get_backtrace()). As far as I can tell this has only one very minor
effect, because that field is only used for those objects to determine how
ThreadResponsiveness::Update() dispatches its runnables.
--HG--
extra : rebase_source : 39318dba51316dc473321d6e4215e5b950a4d41d
Currently ThreadInfo objects all share gBuffer, while SyncProfile objects each
get their own ProfileBuffer.
This patch removes ThreadInfo::mBuffer to reduce this difference, taking us a
step towards eliminating SyncProfile.
To support this, the patch:
- passes in a buffer as an additional argument in a bunch of places where the
buffer used to be obtained from a ThreadInfo;
- adds an mBuffer field to ProfilerBacktrace;
- changes ThreadInfo::SetProfile() to SetHasProfile();
- removes ThreadInfo::{addTag,StoredMarker,bufferGeneration}(), all of which
just redirected to ThreadInfo anyway;
- changes ProfileBuffer so it's no longer refcounted, which is unnecessary now
that gBuffer and ProfilerBacktrace::mBuffer don't have multiple references,
which makes their lifetimes obvious.
The patch also removes some ThreadInfo& args in functions in platform.cpp, in
places where that ThreadInfo is available within the accompanying TickSampler*
arg.
--HG--
extra : rebase_source : 7e6cb370866d3f3fd657c6aa66d3c3eb3d83a4b1
The MacOS and Windows profiler cores have a threading structure where one
thread ("sampler thread") collects information from a thread to be profiled
("samplee thread") by suspending it, getting its register state, unwinding its
stack, and then resuming it. This requires kernel-level primitives to perform
the suspend, get-registers and resume steps.
The Linux/Android core is different, because none of those three primitives
exist. Until now, data collection has been done by sending a SIGPROF to the
samplee, and collecting all relevant data within the signal handler. This has
a number of disadvantages:
(1) Current work to rationalise/clean up the threading structure of the
profiler is complicated by the need to reason about/verify two different
schemes.
In particular, the Tick call in the Windows and MacOS implementations will
produce its output on the sampler thread. In the Linux implementation
that is produced on the sampled threads.
(2) Dynamic verification results (primarily, absence of data races and
deadlocks) established for the Linux implementation are less likely to
carry over to the other two implementations, because the threading
structures are different.
(3) It causes a lot of duplicated code in platform-*.cpp. For example
SamplerThread::Run() in the -win32.cpp and -macos.cpp files are very
similar. Ideally all three could be merged into a single file with the
identical logic commoned up.
(4) Running lots of code -- the entire contents of Tick -- in a signal handler
isn't considered good practice. POSIX severely restricts the set of
functions we can safely call from within a signal handler.
This commit changes the Linux implementation by using semaphores to implement
the suspend and resume primitives, and moves the bulk of the data collection
work to the sampler thread. By doing this, it causes the Linux implementation
to have the same threading structure as the other two.
--HG--
extra : rebase_source : 675b6ef76915d164ed263b831dddd6ce0c0e97f3
There's no need to lock when calling Tick() on a local TickSample that uses a
fresh SyncProfile with its own fresh ProfileBuffer -- none of that data can
be touched by another thread.
--HG--
extra : rebase_source : aaabef89e8481758b566e6dd01e4bb61a5855b1a
Both of these functions are now trivial and identical in both ThreadInfo and
SyncProfile. This patch inlines and removes them.
--HG--
extra : rebase_source : 15fb7c1d4df9fbc80d8e671761b4aa1508845cac
It appears to be a remnant of a time when SyncProfile lifetimes were more
complex. Nowadays they are simple.
- profiler_get_backtrace() constructs a SyncProfile called |profile|.
|profile|'s mOwnerState is REFERENCED.
- profiler_get_backtrace() then calls BeginUnwind() and EndUnwind() on
|profile|. After the EndUnwind(), |profile->mOwnerState| is always OWNED.
- |profile| then is put into the returned ProfilerBacktrace. That
ProfilerBacktrace will destroy |profile| in its destructor because
ShouldDestroy() always returns true because mOwnerState is always OWNED.
The OWNER_DESTROYING and ORPHANED states are never used, and the whole
OwnerState type isn't necessary. This patch removes it and ShouldDestroy().
--HG--
extra : rebase_source : f1828b4a5d6b8f73245e666f457b93a24ca7266e
This patch does the following.
- Uses "entries" consistently for the name of the value that is obtained from
MOZ_PROFILER_ENTRIES and is the first argument to profiler_start(). (I.e. not
"entry" or "entrySize".)
- Removes variables (e.g. PROFILER_HELP) holding env var names and uses the
names (e.g. "MOZ_PROFILER_HELP") directly. Some of the names are already used
directly and I think the slight repetition isn't harmful. It's unlikely that
we'd want to change these names the way we might need to change a numeric
value, and they're perfectly descriptive.
- Changes the "MOZ_PROFILING_FEATURES" string in the weird Android-only startup
code to be "MOZ_PROFILER_FEATURES", for consistency.
- Renames gUnwindInterval and gProfileEntries as gEnvVarInterval and
gEnvVarEntries to make it clearer that they come from environment variables,
but otherwise are parallel to gInterval and gEntries.
- Puts entries before intervals in most places, to match the profiler_start()
argument order.
- Changes profiler_usage() so that (a) it always prints, no matter the
verbosity, (b) it exits at its end, and (c) doesn't double-print "Profiler: "
at the start of each line.
--HG--
extra : rebase_source : e5a0b1c48e390ada894c746f050f08ff5c241066
The |nsIFile*| one is only called by the |const nsACString&| one, so this patch
combines them.
--HG--
extra : rebase_source : d8338e88cef4799d95e590c056ab343d5a1c546a
profiler_get_gatherer() exposes ProfileGatherer to the outside world in a way
that makes future changes difficult.
This patch:
- Removes ProfileGatherer.h from the list of headers exported from the
profiler.
- Removes nsIProfiler.profileGatherer and nsProfiler::GetProfileGatherer().
- Replaces profiler_get_gatherer() with three new functions that provide
minimal but sufficient access to ProfileGatherer:
profiler_will_gather_OOP_profile(), profiler_gathered_OOP_profile(), and
profiler_OOP_exit_profile().
These functions provide access to the ProfileGatherer in a similar fashion to
the pre-existing functions profiler_get_profile_jsobject_async() and
profiler_save_profile_to_file_async()
This significantly reduces the size of the profiler's API surface.
--HG--
rename : tools/profiler/public/ProfileGatherer.h => tools/profiler/gecko/ProfileGatherer.h
extra : rebase_source : d8e06a1133d4098c3a214858d3ff2c4bdcd9f1f2
This removes the one use of gStartTime outside of platform*.cpp, which lets us
restrict its visibility to just that compilation unit.
--HG--
extra : rebase_source : bf7207572cba5c1a31b544ea73e783ecd559978a
PlatformStart() and PlatformStop() are currently responsible for setting and
clearing gIsActive, but it's better if we do it in profiler_{start,stop}().
The patch also does the following.
- Adds some missing emacs/vim modelines.
- Makes Platform{Start,Stop}() crash if they have failures. I'm not at all
confident that ignoring the errors as is currently done will result in
sensible behaviour, so brittleness is better.
--HG--
extra : rebase_source : b9ab8437f5b92f6a8993ba7677ecb74a321ce219
This patch mostly does formatting fixes.
It also removes some declarations from platform.h that are no longer necessary
now that platform-linux-android.cpp is in the same compilation unit as
platform.cpp (due to it being #include-d directly); this required reordering
some things.
--HG--
extra : rebase_source : d07ef71455885fe8f1414d87c261ca054989a6a8
This avoids the need for platform-linux-android.cpp to read gInterval off the
main thread in an awkward spot. It also makes platform-linux-android.cpp
more like platform-{win32,macos}.cpp.
--HG--
extra : rebase_source : c1c76a382d6373f9fd2e3f89a1e1f8fef9072257
There is another PlatformStop() call earlier in the function, and gIsActive is
always false by the time we reach the removed call, so it's dead code.
--HG--
extra : rebase_source : 3b358b6bef47d394d6d6bc76d1153ea38968919e
It's only being used in a boolean fashion, so this patch replaces it with a
boolean.
--HG--
extra : rebase_source : 91152dff81107070fa49b3984e1b6759e0cd6d20
This change means that all the relevant code is now within
platform-linux-android.cpp, which is nice.
--HG--
extra : rebase_source : 886a31005fdb67fae65e6f4209796973f1391244
- Don't bother checking gSampler in ProfilerSignalHandler. It is equivalent
to checking gIsActive and we do that at the top of the loop in
SignalSender(). There is no point repeatedly checking the same condition in
the middle of that loop; that just opens up the possibility of partially
complete samples where some threads are missing.
- Clear gCurrentThreadInfo in SignalSender() instead of in
ProfilerSignalHandler(). The effect is much the same, but this change means
gCurrentThreadInfo is both set and cleared in SignalSender(), i.e. on a
single thread, removing any need for Atomic<>.
--HG--
extra : rebase_source : 645d321de4cad6fdb32383b6f1d0c7cbe54308fc
We don't need OS now that the platform-*.cpp files are in the same compilation
unit as platform.cpp.
The patch removes the sleep functions because they are unnecessary indirection.
OS::Startup() is necessary, but the patch renames it PlatformInit() to match
Platform{Start,Stop}() and profiler_init(), from which it is called.
Specifically:
- platform-linux.cc -> platform-linux-android.cpp
- platform-macos.cc -> platform-macos.cpp
- platform-win32.cc -> platform-win32.cpp
Adding "android" to the first one is the most important part, because it makes
things clearer. The .cc to .cpp change is less important but I might as well do
it while I'm in here.
--HG--
rename : tools/profiler/core/platform-linux.cc => tools/profiler/core/platform-linux-android.cpp
rename : tools/profiler/core/platform-macos.cc => tools/profiler/core/platform-macos.cpp
rename : tools/profiler/core/platform-win32.cc => tools/profiler/core/platform-win32.cpp
extra : rebase_source : 371f91c4cd95e88e1723e192e68f16ba66965c8f
Currently we use the SPS_* macros in some places, but also use other ones like
__arm__ and ANDROID and XP_{WIN,MAC,LINUX}. This patch makes the profiler
consistently use the SPS_* macros and removes the V8_HOST_ARCH_* macros.
The patch also does the following.
- Cleans up some header inclusions, e.g. including pthread.h directly in the
files that use it, and removing some unneeded android/log.h inclusions.
- Removes an unused branch in SetSampleContext() -- we don't support ARM on
anything other than Android, and glibc 2.3 is ancient.
- Doesn't use SPS_* in PseudoStack.h because that would require exporting
PlatformMacros.h, which doesn't seem worthwhile.
Some things that aid the understanding of this patch.
- XP_LINUX and LINUX are both defined for Linux *and* Android.
- x86/Android is the only supported platform that doesn't define
HAVE_NATIVE_UNWIND.
- Every platform that defines USE_LUL_STACKWALK also defines
HAVE_NATIVE_UNWIND.
--HG--
extra : rebase_source : 561b708f9434cabd9c0e00d4f4bfdd53f7008670
It's defined if any of XP_{WIN,MAC,LINUX} are defined and the latter includes
Android as well. So it's defined on all the OSes the profiler supports.
--HG--
extra : rebase_source : 1fa9c1fb573a99375b477a048c0b4575ac1eeca0
They must be set before the call to MaybeSetProfiler(), which checks
gProfileThreads, and before the gTaskTracer check.
This patch fixes the failure of some threads (e.g. Compositor) to be profiled,
as well as allowing TaskTracer to start up again!
This change increases consistency:
- Each OS is dealt with one at a time (no more interleaving).
- For each OS, x86 is now the first listed architecture.
The patch also adds the missing "#undef SPS_PLAT_x86_android".
--HG--
extra : rebase_source : 6ab278ff2523269424aa6dc872ec35ca6dc735a4
They duplicate the equivalent SPS_* macros. (The SPS_* macros have already
crept into use in some places within LUL.)
--HG--
extra : rebase_source : 65ed6e6e147189814511b0ca38342fec118478b9
Instead of calling MaybeShowStats() every 16th time around the inner loop we
now check every time around the outer loop. Because there are typically 20--50
threads running at once, this results in a slightly lower freqency of printing,
but that seems fine because this is debug-only code.
The new names make it clearer that these actions apply to just one thread.
- profiler_sleep_start() --> profiler_thread_sleep()
- profiler_sleep_end() --> profiler_thread_wake()
- profiler_is_sleeping() --> profiler_thread_is_sleeping()
- GeckoProfilerSleepRAII --> GeckoProfilerThreadSleepRAII
- GeckoProfilerWakeRAII --> GeckoProfilerThreadWakeRAII
This patch:
- Improves some comments.
- Adds some assertions.
- Removes unnecessary differences between some Mac and Windows code.
- Removes the unnecessary SamplerThread::mStackSize on Windows.
"unwinder" and "jank" are never checked for via hasFeature(), and are not
mentioned anywhere else in the profiler. This patch removes them from
profiler_get_features().
"restyle" *does* have a hasFeature() call and is handled by
profiler_feature_active(). This patch adds it to profiler_get_features().
Sleep handling is currently done with two counters (mSleepId, mSleepIdObserved)
and a boolean (mSleeping). This patch replaces it with a single tri-state
variable (mSleep), which is enough to replicate the functionality.
The patch also clearly documents how this variable is accessed from multiple
threads and the potential inaccuracies that can result from races.
This patch:
- Reformats PseudoStack.h to more closely follow Mozilla style.
- Rewrites some comments to make them more readable, e.g. by properly
delimiting sentences with upper-case letters and full stops.
- Replaces sMin() with std::min(), because <algorithm> no longer causes
problems.
- Reorders PseudoStack so that all the data members are at the end, which makes
them easier to see.
Sampler's constructor and destructor are inlined into profiler_{start,stop}(),
respectively. A trivial Sampler class is left behind (now in platform.cpp)
because there are still numerous gSampler null checks, which I will remove in a
follow-up bug.
PseudoStack::flushSamplerOnShutdown() moves into platform.cpp. MAXPATHLEN is
unused and is deleted.
Two of them (mAddMainThreadIO and mPrivacyMode) are now local variables within
profiler_start(), which is nice.
This change also lets us get rid of four sIs* variables.
Doing so made it clearer that gThreadNameFilters was being accessed from
multiple threads without synchronization, so I added a Mutex for it.
--HG--
extra : rebase_source : 60d9f4df76ed04cb5ce1b8232bbeb9c795b66535
They don't particularly belong there, and Sampler will be going away eventually
anyway.
--HG--
extra : rebase_source : 4faab111925848215f7e8db8244b33ee46c5396c
I am planning to merge Sampler into platform.cpp, so Sampler.cpp will
disappear. This change will make that easier, because things that temporarily
need to be visible in both files won't need to be declared in a header.
--HG--
extra : rebase_source : f0fa4751f6ead945c1a00a17dbc9a7d3dc870e4b
platform-*.cc and platform.cpp belong together conceptually, and combining them
into a single compilation unit makes it easier to share things and avoids the
need for some declarations in headers.
The patch also removes old_sigsave_signal_handler_ which is a long-dead field
that clang now detects and complains about.
--HG--
extra : rebase_source : 092a7ca608415b888607dba38ad5296787af824e
