If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

clean up telemetry hashtable reflection

RESOLVED FIXED in mozilla13

Status

()

Toolkit
Telemetry
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
We have a very common:

construct a JSObject O
iterate over hashtable to define properties on O
check for failure
return O

pattern in Telemetry.  And it's only going to get more common with bug 715927 and related bugs.  Let's split this out into common code.
(Assignee)

Comment 1

6 years ago
Created attachment 595787 [details] [diff] [review]
patch

This is a much-cut-down version of the patch in bug 715927: no addon histograms, no persistent telemetry bits.  IMHO, it's much easier to do this here and then make those patches use it.
Attachment #595787 - Flags: review?(taras.mozilla)
(Assignee)

Updated

6 years ago
Summary: clean up telemetry hashtable initialization → clean up telemetry hashtable reflection
(Assignee)

Comment 2

6 years ago
Created attachment 595837 [details] [diff] [review]
patch

Second iteration, with AutoHashtable as discussed on IRC.
Attachment #595787 - Attachment is obsolete: true
Attachment #595787 - Flags: review?(taras.mozilla)
Attachment #595837 - Flags: review?(taras.mozilla)

Comment 3

6 years ago
Comment on attachment 595837 [details] [diff] [review]
patch

+  // This gets marked immutable in debug builds, so we can't use
+  // AutoHashtable here.
   nsTHashtable<nsCStringHashKey> mTrackedDBs;

Seems like we shouldn't be using a nsTHashtable for that either. Could probably just keep a list of hashes precompiled at compile-time with some template magic. Where is a perfect-hash lib when you need one :(
Attachment #595837 - Flags: review?(taras.mozilla) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed

Updated

6 years ago
Whiteboard: [autoland-try]

Updated

6 years ago
Whiteboard: [autoland-try] → [autoland-in-queue]

Comment 4

6 years ago
Autoland Patchset:
	Patches: 595837
	Branch: mozilla-central => try
Error applying patch 595837 to mozilla-central.
patching file toolkit/components/telemetry/Telemetry.cpp
Hunk #3 FAILED at 396
1 out of 5 hunks FAILED -- saving rejects to file toolkit/components/telemetry/Telemetry.cpp.rej
abort: patch failed to apply

Could not apply and push patchset:

Updated

6 years ago
Whiteboard: [autoland-in-queue]
(Assignee)

Comment 5

6 years ago
Created attachment 596165 [details] [diff] [review]
patch

Rebasing to something that applies cleanly to m-c.
Attachment #595837 - Attachment is obsolete: true
Attachment #596165 - Flags: review+
(Assignee)

Updated

6 years ago
Whiteboard: [autoland-try]

Updated

6 years ago
Whiteboard: [autoland-try] → [autoland-in-queue]

Comment 6

6 years ago
Autoland Patchset:
	Patches: 596165
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/rev/b5be9d37da7e
Try run started, revision b5be9d37da7e. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=b5be9d37da7e

Comment 7

6 years ago
Try run for b5be9d37da7e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b5be9d37da7e
Results (out of 209 total builds):
    exception: 4
    success: 171
    warnings: 20
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-b5be9d37da7e

Updated

6 years ago
Whiteboard: [autoland-in-queue]
Comment on attachment 596165 [details] [diff] [review]
patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/108f5491cf3f
Attachment #596165 - Flags: checkin+
Target Milestone: --- → mozilla13
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/108f5491cf3f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Depends on: 727081
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.