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

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

({main-thread-io})

unspecified
mozilla26
main-thread-io
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Async])

Attachments

(1 attachment, 1 obsolete attachment)

To simplify the work on moving file I/O off the main thread, we should first move file I/O away from TelemetryPing.js
Created attachment 787002 [details] [diff] [review]
Extracting TelemetryFile.jsm 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
Keywords: checkin-needed
Created attachment 788131 [details] [diff] [review]
Extracting TelemetryFile.jsm from TelemetryPing.js, v2

Applied feedback.
Attachment #787002 - Attachment is obsolete: true
Attachment #788131 - Flags: review+
Keywords: checkin-needed
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
Last Resolved: 5 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.