Add profile label to address encoding pipeline performance issue in pseudostack. https://developer.mozilla.org/en-US/docs/Performance/Profiling_with_the_Built-in_Profiler

Free this if anyone wants.

Hi Ehsan, Sorry to bother you. We tried to add this label on "Media Encoder" thread but we still can't find this tag shown on cleopatra. Did I miss something? Should I add this thread name on somewhere?

(In reply to Randy Lin [:rlin] from comment #2) > Hi Ehsan, > Sorry to bother you. > We tried to add this label on "Media Encoder" thread but we still can't find > this tag shown on cleopatra. > Did I miss something? Should I add this thread name on somewhere? Can I see a patch? :)

Did you call profiler_register_thread?

I just have a test on this but got a crash on F/MOZ_Assert( 6757): Assertion failure: false, at ../../../../../../media/ssd/gecko_c2/tools/profiler/platform-linux.cc:465 Did I miss something?

Fix incorrect profiler_unregister_thread location. But I still get the same crash problem...

(In reply to Randy Lin [:rlin] from comment #5) > Created attachment 8496723 [details] [diff] [review] > test patch 1 > > I just have a test on this but got a crash on > F/MOZ_Assert( 6757): Assertion failure: false, at > ../../../../../../media/ssd/gecko_c2/tools/profiler/platform-linux.cc:465 > > Did I miss something? Did you read the comment right above that MOZ_ASSERT(false)? <http://mxr.mozilla.org/mozilla-central/source/tools/profiler/platform-linux.cc#463> It clearly explains what's happening here. You're registering the same thread multiple times.

Hi, Ehasn I add some profiling label in the media encoding pipeline, could you help me review my patch? Thanks :)

It's my profiling result.

Comment on attachment 8499472 [details] [diff] [review] Patch v1 Review of attachment 8499472 [details] [diff] [review]: ----------------------------------------------------------------- Nit: please create the patches with 8 lines of context for review. Thanks! On the patch itself, the PROFILER_LABEL additions themselves look fine, but I don't know this code very well, so I don't feel comfortable reviewing this. Things to note during the review is the threading model (see the comment below), and also, whether any of these functions can be hot (since pushing and poping these labels can be costly, and therefore adding too many labels may skew the profile.) Perhaps try cpearce? ::: content/media/MediaRecorder.cpp @@ +439,4 @@ > MOZ_ASSERT(NS_GetCurrentThread() == mReadThread); > LOG(PR_LOG_DEBUG, ("Session.Extract %p", this)); > > + if (!mIsRegisterProfiler) { I am not sure about the threading model of the code involved here, but please make sure that this doesn't race, i.e., there can only ever be one thread entering this code for the first time. Otherwise you'll have a race condition on two threads trying to call profiler_register_thread. ::: content/media/encoder/fmp4_muxer/ISOMediaWriter.h @@ +8,4 @@ > > #include "ContainerWriter.h" > #include "nsIRunnable.h" > +#include "GeckoProfiler.h" Nit: please move this to the .cpp file, so that all consumers of this header don't pull in GeckoProfiler.h needlessly.

Comment on attachment 8499472 [details] [diff] [review] Patch v1 Review of attachment 8499472 [details] [diff] [review]: ----------------------------------------------------------------- I don't know this code very well, so I'd prefer you to find someone else to look at it. Probably roc is the best bet. He reviewed most of MediaRecorder I think.

Hi, Robert. Sorry to bother you, could you help me review my patch? Thanks!

Comment on attachment 8500215 [details] [diff] [review] Patch v2 Switch to review flag. :)

Add the profiler_unregister_thread() and remove some labels in MediaRecorder.

Hi Alastor, could you provide a try run to make sure everything works and nothing breaks. Thanks!

(In reply to Carsten Book [:Tomcat] from comment #16) > Hi Alastor, > > could you provide a try run to make sure everything works and nothing breaks. > > Thanks! Here is the try-server record, thanks! https://tbpl.mozilla.org/?tree=Try&rev=ce204a31ed28