Closed Bug 972577 Opened 6 years ago Closed 6 years ago

Consolidate IOInterposer code

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: aklotz, Assigned: aklotz)

References

Details

Attachments

(1 file, 2 obsolete files)

Right now the IOInterposer code is scattered about multiple places in the tree.

We've got 3 separate code paths for its initialization: profiler, telemetry, and late write checks.

Most of the code is in tools/profiler but it is now relying on some code in xpcom/build.

I'd like to clean all of this up so that we can condense this down to one location for IOInterposer code and allow us to initialize IOInterposer in a  more coherent way.

For example, now that PoisonIOInterposerWin is enabled I don't think that we should initialize NSPRInterposer on Windows anymore, since the latter captures a subset of the I/O captured by the former.

I think that this will help to mitigate problems like https://github.com/bgirard/Gecko-Profiler-Addon/issues/73

For example, on Windows we are potentially generating 2 I/O observations for each single I/O operation: one for NSPRInterposer and one for PoisonIOInterposerWin. That means that we are stackwalking the same I/O event twice and generating twice the data.
Summary: Colsolidate IOInterposer code → Consolidate IOInterposer code
Attached patch Patch v1 (obsolete) — Splinter Review
All code related to IOInterposer and all function interception code to xpcom/build (since late write poisoning and the PoisonIOInterposer* code already reside there). Feel free to suggest a different location.

Adding Benoit for the changes affecting the profiler and Jim for the Metro change.
Attachment #8390830 - Flags: review?(nfroyd)
Attachment #8390830 - Flags: review?(jmathies)
Attachment #8390830 - Flags: review?(bgirard)
Comment on attachment 8390830 [details] [diff] [review]
Patch v1

Review of attachment 8390830 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/xre/nsAppRunner.cpp
@@ +4025,5 @@
>    char aLocal;
>    GeckoProfilerInitRAII profilerGuard(&aLocal);
>    PROFILER_LABEL("Startup", "XRE_Main");
>  
> +  mozilla::IOInterposerInit ioInterposerGuard;

This isn't on the gecko main thread, is that ok?
(In reply to Jim Mathies [:jimm] (away 4/1-4/21) from comment #2)
> Comment on attachment 8390830 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8390830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/xre/nsAppRunner.cpp
> @@ +4025,5 @@
> >    char aLocal;
> >    GeckoProfilerInitRAII profilerGuard(&aLocal);
> >    PROFILER_LABEL("Startup", "XRE_Main");
> >  
> > +  mozilla::IOInterposerInit ioInterposerGuard;
> 
> This isn't on the gecko main thread, is that ok?
It doesn't have to be on the thread that is considered the Gecko Main Thread, but it should be on the first thread running in the process.
Flags: needinfo?(jmathies)
Comment on attachment 8390830 [details] [diff] [review]
Patch v1

Review of attachment 8390830 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Aaron Klotz [:aklotz] from comment #3)
> (In reply to Jim Mathies [:jimm] (away 4/1-4/21) from comment #2)
> > Comment on attachment 8390830 [details] [diff] [review]
> > Patch v1
> > 
> > Review of attachment 8390830 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/xre/nsAppRunner.cpp
> > @@ +4025,5 @@
> > >    char aLocal;
> > >    GeckoProfilerInitRAII profilerGuard(&aLocal);
> > >    PROFILER_LABEL("Startup", "XRE_Main");
> > >  
> > > +  mozilla::IOInterposerInit ioInterposerGuard;
> > 
> > This isn't on the gecko main thread, is that ok?
> It doesn't have to be on the thread that is considered the Gecko Main
> Thread, but it should be on the first thread running in the process.

That is the case, this is on the app main thread which gets consumed by the runtime, and a new thread is handed back to us which we make the gecko thread.
Attachment #8390830 - Flags: review?(jmathies) → review+
Comment on attachment 8390830 [details] [diff] [review]
Patch v1

Review of attachment 8390830 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is OK, though I'd like an explanation of why NS_IsMainThread isn't the right thing to use, even if that's just "because we do weird things with saying who is the main thread."

::: storage/src/TelemetryVFS.cpp
@@ +106,5 @@
>      if (id != Telemetry::HistogramCount) {
>        Telemetry::AccumulateTimeDelta(static_cast<Telemetry::ID>(id + mainThread),
>                                       start, end);
>      }
> +#if defined(MOZ_ENABLE_PROFILER_SPS) && !defined(XP_WIN)

Can you add a comment here about why !defined(XP_WIN)?

::: toolkit/components/telemetry/Telemetry.cpp
@@ +385,5 @@
>   
>  void TelemetryIOInterposeObserver::Observe(Observation& aOb)
>  {
>    // We only report main-thread I/O
> +  if (!IsMainThread()) {

Can you elaborate on why we have a separate IsMainThread() call now?  Does NS_IsMainThread not do the right thing because of metro?
Attachment #8390830 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #5)
> Can you elaborate on why we have a separate IsMainThread() call now?  Does
> NS_IsMainThread not do the right thing because of metro?

NS_IsMainThread works fine on metro provided you're past xpcom init.
Flags: needinfo?(jmathies)
(In reply to Jim Mathies [:jimm] (away 4/1-4/21) from comment #6)
> (In reply to Nathan Froyd (:froydnj) from comment #5)
> > Can you elaborate on why we have a separate IsMainThread() call now?  Does
> > NS_IsMainThread not do the right thing because of metro?
> 
> NS_IsMainThread works fine on metro provided you're past xpcom init.

I guess we need separate main thread detection if we're catching IO that happens before xpcom init?
Flags: needinfo?(aklotz)
(In reply to Nathan Froyd (:froydnj) from comment #7)
> 
> I guess we need separate main thread detection if we're catching IO that
> happens before xpcom init?
Exactly.
Flags: needinfo?(aklotz)
(In reply to Aaron Klotz [:aklotz] from comment #8)
> (In reply to Nathan Froyd (:froydnj) from comment #7)
> > 
> > I guess we need separate main thread detection if we're catching IO that
> > happens before xpcom init?
> Exactly.

OK.  Can you write a comment to that effect for the declaration of IsMainThread() in IOInterposeObserver?
Attachment #8390830 - Flags: review?(bgirard) → review+
Attached patch Patch v2 (obsolete) — Splinter Review
Added requested comments. Carrying forward r+.
Attachment #8390830 - Attachment is obsolete: true
Attachment #8391430 - Flags: review+
Attached patch Patch v3Splinter Review
Fix for non-unified build bustage. Trivial fix, carrying forward r+
Attachment #8391430 - Attachment is obsolete: true
Attachment #8391536 - Flags: review+
Flags: needinfo?(aklotz)
Depends on: 983957
https://hg.mozilla.org/mozilla-central/rev/c8641d02a06d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.