888534 - IOInterposer enhancements

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement, P3)

Description

[(No longer employed by Mozilla) Aaron Klotz]

Add support for off-main-thread I/O.
Also add support for file create/open and close operations.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: Add new operations for create, stat, close

This patch adds new IOInterposer operations and modifies the SQLite and NSPR interposing accordingly.

Interposing PR_Open is a special case for which I will be attaching a second patch.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2 WIP

WIP for part 2. On Try, JP goes orange on OSX. Investigating.

Comment 3

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 808872 [details] [diff] [review]
Part 1: Add new operations for create, stat, close

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

So I imagined `IOInterposeObserver::Observer` as a small set of generic operation types. But with fsync already
there I see that it makes sense to just make a flag for each type of operation we interpose.
Even if we at some point interpose readdir, rename, remove/unlink and friends, we're probably not going to run out of flags.

Anyways, this looks good to go, so I'll r+. I do have two small things for consideration... But we could also address those
in another patch, or simply not do anything about them at all. Once we have poison IO interposer and filename support we'll
have to discuss whether more metadata is desired in the profiler (assuming that bug on adding metadata is fixed)...

(So remarks below, is more to open a discussion, don't let them stop you from landing this).

::: tools/profiler/IOInterposer.h
@@ +30,2 @@
>      OpWriteFSync = (OpWrite | OpFSync),
> +    OpAll = (OpCreateOrOpen | OpRead | OpWrite | OpFSync | OpStat | OpClose)

Maybe we should add a `OpAllInput` for all input operations, and `OpAllOutput` for all output operations (similar to how OpAll works).
That way a consumer like late-write checks would be able to get all output operations, without a need to update it when new functions are interposed.
(This would replace OpWriteFSync which isn't very generic).

Anyways, just a thought... I could also do it when I rework the late-write checks patch.

::: tools/profiler/ProfilerIOInterposeObserver.cpp
@@ +12,5 @@
> +  const char* str = nullptr;
> +
> +  switch (aObservation.ObservedOperation()) {
> +    case IOInterposeObserver::OpCreateOrOpen:
> +      str = "create/open";

Maybe we should put this in a static function in IOInterposer.(h|cpp).
Ie. have IOInterposerObserver::OperationToString(Operation aOp), as
a static method that gives us a human readable string from an Operation.
(This could be useful elsewhere, and be easier to update when we interpose other operations).

Though an even better solution would be to provide a sensible string
for the IOInterposeObserver::Observation::Reference() property.
This should be done when we report the operation, where we currently just report "NSPRIOInterposing", "sqlite-mainthread" or "sqlite-otherthread".

Anyways, I think this is fine for now. We'll want to look into this again, when PoisonIOInterposer lands...

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: Add new operations for create, stat, close (rebased)

Rebased part 1, carrying forward r+

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Try build:
https://tbpl.mozilla.org/?tree=Try&rev=7bd8b11d45d4

https://hg.mozilla.org/integration/mozilla-inbound/rev/22da748201eb

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/22da748201eb

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Add support for PR_Open to NSPRInterposer

This patch intercepts PR_Open by defining our own version of PR_Open in xul.dll that then loads the real symbol from NSPR. There is a special code path for MacOS that ensures that we provide an absolute path to the dynamic library unless DYLD_LIBRARY_PATH is defined. This is needed to prevent Mn and JP test failures.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8340045 [details] [diff] [review]
Part 2: Add support for PR_Open to NSPRInterposer

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

::: tools/profiler/NSPRInterposer.cpp
@@ +302,5 @@
> +
> +} // anonymous namespace
> +
> +// This works because this symbol is provided to the linker before NSPR.
> +// (This is non-standard but works on all first-tier linkers)

I very much doubt this is true:
- nspr internally calls PR_Open.
- nss calls PR_Open and is linked against nspr, not libxul
- except for linux, symbol binding at dynamic linking time is made on the tuple (lib, symbol), not symbol alone, so anything linked against (nspr, PR_Open) is going to use PR_Open from nspr, not this one.
- even on linux, nspr and nss are linked before libxul (libxul depending on them), which means they are both going to use nspr's PR_Open. Furthermore, binary components (like libbrowsercomps.so), linked against nspr, and libxul, are likely to use nspr's PR_Open.

I think the best thing to do here is to make an ABI compatible change to NSPR that allows to interpose PR_Open like the other PR_* I/O-related functions.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1.5: Add new operations to IOInterposer::Unregister

I missed adding the new operations to IOInterposer::Unregister when I wrote part 1. This patch rectifies that situation. :-)

Comment 10

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8361512 [details] [diff] [review]
Part 1.5: Add new operations to IOInterposer::Unregister

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

Simply perfect, r+
I have no further comments :)

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

https://tbpl.mozilla.org/?tree=Try&rev=be6922557c0e
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a424ccdb347

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7a424ccdb347

Comment 13

[Markus Stange [:mstange]]

Aaron, I'm currently going through all bugs in the Gecko Profiler component and determining which ones we can close.
It looks like all that's left in this bug is the part 2 patch. Do you still have plans for this patch? I suspect it's just for instrumenting off-main-thread IO caused by loading libraries?

Comment 14

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Should check if this is still needed...

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

This is not currently on my radar, but feel free to pick it up if necessary. Part 2 is a bit thorny because we'd have to make some significant changes how PR_Open works in NSPR.

Comment 16

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Still on my NI radar, but low priority.

Comment 17

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

I believe we're intercepting most IOs now, and showing them as markers when one of the "FileIO..." profiler features is enabled.
NtClose is still missing, see bug 1762830.

So I'll mark this old bug here "fixed".