Closed Bug 902434 Opened 11 years ago Closed 11 years ago

[Telemetry] Separate file I/O from the rest of the code

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

(Keywords: main-thread-io, Whiteboard: [Async])

Attachments

(1 file, 1 obsolete file)

To simplify the work on moving file I/O off the main thread, we should first move file I/O away from TelemetryPing.js
Assignee: nobody → dteller
Attachment #787002 - Flags: review?(nfroyd)
Comment on attachment 787002 [details] [diff] [review]
Extracting TelemetryFile.jsm from TelemetryPing.js

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

WFM.  I think everything got moved around correctly.

::: toolkit/components/telemetry/TelemetryFile.jsm
@@ +19,5 @@
> +const PR_CREATE_FILE = 0x8;
> +const PR_TRUNCATE = 0x20;
> +const PR_EXCL = 0x80;
> +const RW_OWNER = 384;
> +const RWX_OWNER = 448;

If you are doing this so that you avoid warnings about literal octal constants, could you please make these use |parseInt("...", 8)|?

::: toolkit/components/telemetry/TelemetryPing.js
@@ +907,5 @@
>    },
>  
>    cacheProfileDirectory: function cacheProfileDirectory() {
> +    // This method doesn't do anything anymore
> +    return;

If this is true, can you file a followup bug on deleting this?
Attachment #787002 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #2)
> Comment on attachment 787002 [details] [diff] [review]
> Extracting TelemetryFile.jsm from TelemetryPing.js
> 
> Review of attachment 787002 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> WFM.  I think everything got moved around correctly.
> 
> ::: toolkit/components/telemetry/TelemetryFile.jsm
> @@ +19,5 @@
> > +const PR_CREATE_FILE = 0x8;
> > +const PR_TRUNCATE = 0x20;
> > +const PR_EXCL = 0x80;
> > +const RW_OWNER = 384;
> > +const RWX_OWNER = 448;
> 
> If you are doing this so that you avoid warnings about literal octal
> constants, could you please make these use |parseInt("...", 8)|?

Will do.

> ::: toolkit/components/telemetry/TelemetryPing.js
> @@ +907,5 @@
> >    },
> >  
> >    cacheProfileDirectory: function cacheProfileDirectory() {
> > +    // This method doesn't do anything anymore
> > +    return;
> 
> If this is true, can you file a followup bug on deleting this?

I'll do that as part of moving it OMT.

At the moment, I can't land stuff because of the following error:
 0:06.16 TEST-PASS | /Users/david/Documents/Code/mc-telemetry/obj-x86_64-apple-darwin11.4.2/_tests/xpcshell/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js | [checkPayload : 296] true == true
 0:06.16 /Users/david/Documents/Code/mc-telemetry/memory/mozjemalloc/jemalloc.c:6767: Failed assertion: "stats->mapped >= stats->allocated + stats->waste + stats->page_cache + stats->bookkeeping"
 0:06.16 Hit MOZ_CRASH() at /Users/david/Documents/Code/mc-telemetry/memory/build/jemalloc_config.c:33

Does this sound familiar to you?
(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> At the moment, I can't land stuff because of the following error:
>  0:06.16 TEST-PASS |
> /Users/david/Documents/Code/mc-telemetry/obj-x86_64-apple-darwin11.4.2/
> _tests/xpcshell/toolkit/components/telemetry/tests/unit/test_TelemetryPing.
> js | [checkPayload : 296] true == true
>  0:06.16
> /Users/david/Documents/Code/mc-telemetry/memory/mozjemalloc/jemalloc.c:6767:
> Failed assertion: "stats->mapped >= stats->allocated + stats->waste +
> stats->page_cache + stats->bookkeeping"
>  0:06.16 Hit MOZ_CRASH() at
> /Users/david/Documents/Code/mc-telemetry/memory/build/jemalloc_config.c:33
> 
> Does this sound familiar to you?

I have no idea what that is about.  glandium?
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(mh+mozilla)
That's my code, and that's pretty bad.

I think you can proceed without fixing this.  jemalloc does not run on our tinderboxen except in release builds, which don't have this assertion.  For running locally, you can just comment this out.

Would you mind filing a new bug on this with STR and assigning it to me?
Justin: I'll do that, thanks.

In the meantime, here's the Try: https://tbpl.mozilla.org/?tree=Try&rev=abb78705c645
Let's try to check this in.
Keywords: checkin-needed
Applied feedback.
Attachment #787002 - Attachment is obsolete: true
Attachment #788131 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/12c02882eff6
Keywords: checkin-needed
Whiteboard: [Async] → [Async][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/12c02882eff6
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Async][fixed-in-fx-team] → [Async]
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: