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)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: Yoric, Assigned: Yoric)
References
Details
(Keywords: main-thread-io, Whiteboard: [Async])
Attachments
(1 file, 1 obsolete file)
24.81 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•11 years ago
|
||
Assignee: nobody → dteller
Attachment #787002 -
Flags: review?(nfroyd)
Comment 2•11 years ago
|
||
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+
Assignee | ||
Comment 3•11 years ago
|
||
(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?
Comment 4•11 years ago
|
||
(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)
Updated•11 years ago
|
Flags: needinfo?(mh+mozilla)
Comment 5•11 years ago
|
||
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?
Assignee | ||
Comment 6•11 years ago
|
||
Justin: I'll do that, thanks. In the meantime, here's the Try: https://tbpl.mozilla.org/?tree=Try&rev=abb78705c645
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 8•11 years ago
|
||
Applied feedback.
Attachment #787002 -
Attachment is obsolete: true
Attachment #788131 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 9•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/12c02882eff6
Keywords: checkin-needed
Whiteboard: [Async] → [Async][fixed-in-fx-team]
Comment 10•11 years ago
|
||
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.
Description
•