Last Comment Bug 726761 - deserializing Histogram SampleSets shouldn't check count sums
: deserializing Histogram SampleSets shouldn't check count sums
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Nathan Froyd [:froydnj]
:
: [PTO to Dec5] Bill McCloskey (:billm)
Mentors:
Depends on:
Blocks: 707320
  Show dependency treegraph
 
Reported: 2012-02-13 13:43 PST by Nathan Froyd [:froydnj]
Modified: 2012-02-20 10:22 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.31 KB, patch)
2012-02-13 13:48 PST, Nathan Froyd [:froydnj]
cjones.bugs: review+
Details | Diff | Splinter Review
patch (1.02 KB, patch)
2012-02-16 06:38 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review

Description Nathan Froyd [:froydnj] 2012-02-13 13:43:42 PST
See bug 707320 comment 57 for the background here.
Comment 1 Nathan Froyd [:froydnj] 2012-02-13 13:48:21 PST
Created attachment 596781 [details] [diff] [review]
patch

Deserialization shouldn't care whether the data "looks good"; that determination should be left to someone else.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-13 16:12:01 PST
Comment on attachment 596781 [details] [diff] [review]
patch

AIUI this check caught a bug in our code.  Why do you want to remove it?  If you want to change the invariants on histogram state, then do that but update the check here.
Comment 3 Nathan Froyd [:froydnj] 2012-02-15 04:10:07 PST
We check the "invariants" on histogram state elsewhere (e.g. calling FindCorruption on histograms before reflecting them into JS and sending them off).  It's not useful to have a check in the deserialization code for this sort of thing because it lands us in the following situation:

- Histogram X existed in memory with (very rare!) differing counts and we happily included/ignored it when sending telemetry pings.
- Histogram X gets serialized at shutdown.
- Histogram X gets deserialized at startup and we think the datafile is corrupt because of this check.

Without the check, we would include/ignore histogram X just as we were doing before.
Comment 4 Nathan Froyd [:froydnj] 2012-02-15 05:07:26 PST
We can also run into the following scenario:

- Histogram X existed in memory with differing counts and we happily included it when sending telemetry pings (because it's not corrupt according to Histogram::FindCorruption).
- Histogram X gets serialized at shutdown.
- Firefox gets upgraded and histogram X does not exist in the new version of Firefox: we decided it was no longer useful, or we are measuring something slightly different and decided to rename it.
- When reading in histogram X's data, we notice that it's "corrupt" and therefore decide the whole file is corrupt.  We do this *even though* we don't care about the contents of the data (limitations in the current implementation dictate that we can only send serialized histograms that exist in TelemetryHistogram.h), we just want to read it off the disk.

Checking for corrupt data should be put at a higher level than this.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-15 22:33:40 PST
Comment on attachment 596781 [details] [diff] [review]
patch

Just remove all this code.  Hopefully the scheme you describe in the above two comments is documented elsewhere, so we shouldn't need this comment.

r=me with that
Comment 6 Nathan Froyd [:froydnj] 2012-02-16 06:38:04 PST
Created attachment 597795 [details] [diff] [review]
patch

Address review comments by deleting comments.  Carrying over r+.
Comment 7 Mozilla RelEng Bot 2012-02-16 06:44:01 PST
Autoland Patchset:
	Patches: 597795
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=898595eb9cbf
Try run started, revision 898595eb9cbf. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=898595eb9cbf
Comment 8 Mozilla RelEng Bot 2012-02-16 10:46:10 PST
Try run for 898595eb9cbf is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=898595eb9cbf
Results (out of 212 total builds):
    exception: 1
    success: 175
    warnings: 22
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-898595eb9cbf
Comment 10 Ed Morley [:emorley] 2012-02-20 10:22:55 PST
https://hg.mozilla.org/mozilla-central/rev/da1dae1b215d

Note You need to log in before you can comment on or make changes to this bug.