Closed Bug 832076 Opened 11 years ago Closed 5 years ago

Use Write Poisoning to show IO Writes

Categories

(Core :: Gecko Profiler, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: BenWa, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Example:
http://people.mozilla.com/~bgirard/cleopatra/#report=3b47079a09c0d1bb3a22190ca22d0e247bf9322d

With part 1 + a temporary change in the profiler to enable the write poisoning we get markers when a thread does main thread IO but we don't get the stack trace. To support this we need to refactor the stack interwinding and finish bug 809317.

Let's land part 1 while we wait for bug 809317.
Attachment #703648 - Flags: review?(respindola)
Comment on attachment 703648 [details] [diff] [review]
Part 1: Add support for poisoning writes for different purposes

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

I like the idea a lot.  My main concern is how we represent the state we are in.  We already have a variable representing the three poisoning states (uninitialized, poisoned, shutting down). I think we just need one more saying what do we do with an write when we catch it. If you don't mind, we can even drop the MOZ_ASSERT in this patch and the possible actions become

* crash
* record a late write
* ignore
* let the profiler know about it

Also note that this catches *all* writes, not just the ones from the main thread.

::: xpcom/build/mozPoisonWriteBase.cpp
@@ +106,5 @@
>  }
>  
> +bool sTelemetryLateWrite = false;
> +bool sProfilerIO = false;
> +

Can this be a single variable of enum type saying "what are we doing with writes"?

@@ +112,5 @@
>  {
>      // On a debug build, just crash.
>      MOZ_ASSERT(ok);
>  
> +    printf("Write\n");

Leftover debug?

@@ +114,5 @@
>      MOZ_ASSERT(ok);
>  
> +    printf("Write\n");
> +    if (sProfilerIO) {
> +      SAMPLE_MARKER("IO");

You probably want this before the assert. Or more likely, you don't want to assert at all if we get here for for sampling.
Attachment #703648 - Flags: review?(respindola) → review-
We discussed this on IRC and BenWa convinced me the profiling state should be orthogonal to what Telemetry is doing. A user without telemetry can still want to use the profiler to debug shutdown for example.

My request then changes directions, instead of unifying the boolean flags, expand the API so that profiling notifications can be enabled or disabled independently from what Telemetry is doing.
Blocks: 1329137

I believe this work is no longer relevant to currently planned disk io features.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.