Closed Bug 1425909 Opened 2 years ago Closed 2 years ago

Allow adding JavaScript-only scalars in artifact builds

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: gfritzsche, Assigned: Dexter)

References

Details

Attachments

(3 files)

With the new infrastructure we have in place to record probes from add-ons [1], we can use this now to enable JS-only Firefox probes that don't require building C++ code.


1: https://medium.com/georg-fritzsche/recording-new-telemetry-from-add-ons-61d194568212
I sketched this out on the flight back from Austin, working through scalars first.

The basic idea is to add a separate file for registering JS scalars (JSScalars.yaml).
In it, you register the probe, similar to Scalars.yaml:
> browser.toolbar
>   open_count:
>     kind: uint
>     release_channel_collection: opt-out
>     ...

Doing a Firefoy build (artifact, faster or full) will generate a file JSScalars.jsm.
This contains the probe information in a format that `nsITelemetry.registerScalars()` [2] can understand:
> this.JSScalars = {
>   "browser.toolbar": {
>     "open_count": {
>       "kind": Telemetry.SCALAR_TYPE_COUNT,
>       "record_on_release": true,
...

Then when Firefox starts up, we load that file (e.g. from TelemetryController.jsm) and register the scalars.
> registerJsProbes() {
>   for (let [category, scalars] of Object.entries(JSScalars)) {
>     Telemetry.registerBuiltinScalars(category, scalars);
>   }
> },

Subsequently these scalars can be recorded using the same APIs that we are already used to:
> services.telemetry.scalarAdd("browser.toolbar.open_count", 1);

The obvious trade-offs here are:
1) Startup performance from registration calls and loading additional JS modules very early.
2) Memory footprint from loading additional JS modules?
3) Run-time performance from hitting the dynamic recording paths.
We should measure 1) if this is a concern.
I guess 3) is a weak point: If this probe is in a performance sensitive path (being called often), it can just be moved to Scalars.yaml, without changing the format or anything. Or you just shouldn't do JS in the first place.

@Gijs: How does that design look to you, both as an engineer that uses probes and a Firefox engineer that needs our Fireofx startup performance to be good?

Notes on Telemetry internals:
- This is using a separate function `registerBuiltinScalars()` so that we flag them to show up in "parent", "content", ... in the "main" ping payload and not in the "dynamic" section.
- I'm using a separate file `JSScalars.yaml` because:
  a) I don't know how to avoid building the C++ headers if these live in the normal Scalars.yaml. Not sure if that's needed?
  b) We were considering modularizing the build more anyway. Not sure if that's a good argument right now vs. doing a "JS only" flag in Scalars.yaml.
  3) Yuck, test_TelemetryController.js doesn't seem to run all the tasks to the end? Need to investigate more.
- Having it in a separate file depends on us doing a probe-info service (finally) or stubbing out pulling from more files in ETL jobs. Something to talk through.

@Dexter, @chutten: How does this design look to you from the Telemetry perspective, both client & upstream like TMO?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(chutten)
Flags: needinfo?(alessio.placitelli)
Attachment #8937509 - Attachment description: Allow adding new Telemetry probes without building C++ → (failing sketch) Allow adding new Telemetry probes without building C++
This is intriguing. I think it'll take a while to bounce around in my brain fully, but here are some initial thoughts.

* We could incur the runtime registration cost on the first scalarAdd that doesn't find a non-builtin scalar to service it. That ought to offload some of the initial runtime processing to the latest possible time. As a downside, it means the location in time of the runtime registration cost is unpredictable.

* Or, we could link in a native format parser and read it directly into gDynamicScalarInfo, which could allay most of the perf fears.

* Either way, these will suffer all the perf problems of dynamic scalars (linear lookup, string comparisons) but have the potential to be incredibly popular.

* Maybe we can make this a build-faster-only dev feature? Make it so non-build-faster builds append JSScalars.yaml to Scalars.yaml, but allow development to just build frontend code and suffer the runtime cost? (Analogous a little to debug/release builds in this way)
Flags: needinfo?(chutten)
(In reply to Chris H-C :chutten from comment #3)
> * Maybe we can make this a build-faster-only dev feature? Make it so
> non-build-faster builds append JSScalars.yaml to Scalars.yaml, but allow
> development to just build frontend code and suffer the runtime cost?
> (Analogous a little to debug/release builds in this way)

That's an interesting point.
I'm slightly worried about using different code paths, but if this has relevant perf impacts that might trump it.
I wonder though if an iteration like this is problematic when adding/changing probes: "mach build" -> -> "mach build faster" ...
All else fails we could limit the jsm-based load to artifact builds. It means those of us with full checkouts will miss out on the build benefits, though. But these days it looks fairly cheap to switch between artifact builds and full builds (just a couple of lines in .mozconfig) so maybe there are possible workflows...
I've been wrestling with similar questions for prefs.

> * Either way, these will suffer all the perf problems of dynamic scalars (linear lookup, string comparisons) but have the potential to be incredibly popular.

That sounds like prefs in a nutshell. I'm not sure it qualifies as a successful end state :(
Glad to see this finally happening, great work!

(In reply to Chris H-C :chutten from comment #5)
> All else fails we could limit the jsm-based load to artifact builds. It
> means those of us with full checkouts will miss out on the build benefits,
> though. But these days it looks fairly cheap to switch between artifact
> builds and full builds (just a couple of lines in .mozconfig) so maybe there
> are possible workflows...

I second that. Assuming we have two kind of workflows (I'm not sure if that's exactly the case though):

- Firefox front-end/JS devs who have a faster write code/build/test cycle (as no C++ building and linkage is involved);
- devs who work on c++ parts that are used to "wait some time"  when building stuff.

I would say that the faster/artifact builds are primarily geared toward the first workflow. If that's the case, I think it would be perfectly ok to limit this to be a build-faster-dev-only feature.

Ideally, I would see this feature working like that:

- Users add the scalars to some registry file (not necessarily Scalars.yaml).
- When doing artifact builds, we automatically generate an intermediate JSM file (dev builds only).
- We load it and register scalars as you did, from the JSM file (dev builds only).
- We enforce the usual c++ generation for non dev builds (not sure how).

What concerns me is the difference between dev and non-dev builds, which could be a potential source of hard to debug problems.
However this should be mitigated if we enforce the C++ header generation for non dev builds (which gets treeherder test coverage).
Flags: needinfo?(alessio.placitelli)
(In reply to Alessio Placitelli [:Dexter] from comment #7)
> What concerns me is the difference between dev and non-dev builds, which
> could be a potential source of hard to debug problems.
> However this should be mitigated if we enforce the C++ header generation for
> non dev builds (which gets treeherder test coverage).

Exactly. The way I think of it is that it's the same as not building on every platform before submitting your change. At some point in Firefox development you abruptly notice that there are build configurations that are different, but no less important, than the one you tested on.
(In reply to Chris H-C :chutten from comment #5)
> All else fails we could limit the jsm-based load to artifact builds. It
> means those of us with full checkouts will miss out on the build benefits,
> though. But these days it looks fairly cheap to switch between artifact
> builds and full builds (just a couple of lines in .mozconfig) so maybe there
> are possible workflows...

I think this is where we should go. Also for prefs, in fact. Basically, I think we should end up in a state where:

1) artifact builds should allow adding prefs/telemetry
2) prefs/telemetry are fast [on our shipped builds]
3) API usage from JS should be transparent (ie something vaguely like telemetry.scalarAdd(MY_SCALAR, 4) should work both in artifact builds and non-artifact builds, the difference would be limited to "real" builds being faster than artifact ones in executing that call. Pre-processing could potentially achieve this goal. If it ends up requiring us to preprocess every JS file for artifact builds, though, that doesn't really work... so some thought would have to go in how to architect this.)

The compromises of this solution, as far as I can tell, include:

1) modifying existing telemetry/preferences *may* require a full-on non-artifact rebuild. Kind of depends on how this is implemented, and what kind of modification you need.
2) there's a perf penalty for dynamically added prefs/telemetry on artifact builds.

I think (1) is an acceptable trade-off here.

The perf penalty, based on my experience with prefs, isn't too terrible, certainly not compared to the opt/debug difference. The vast majority of local artifact builds are opt ones, so I wouldn't worry too much about this.
Flags: needinfo?(gijskruitbosch+bugs)
(1) is made simpler by how existing Histograms (except for categorical) and Scalars and Events are immutable, so the only change necessitating a full rebuild is removal. (And those patches are often simple enough to fire off to try without a local build if we want to be cheeky).

I wonder if we can perform a special "removal-only" build. I mean, it'll still be in the enums in C++, there's no getting around that... but maybe we can do all the parsing and validation of the json/yaml to make sure the removal of the probe worked.

Forcing the artifact build to generate the C++ enum files wouldn't be worthless in this case.
Blocks: 831104
We will need to find some help with the build specifics to get this working this quarter.
Building up on the comments above, I have a technical proposal to try to solve this.

- when in "artifact" mode, in addition to the other pre-built files, we also should also ship a copy of the original "Scalars.yaml" file somewhere in the object directory (or where all the artifacts are stored);
- when building in "artifact" mode we parse both the original (in the object dir) and the one in toolkit/components/telemetry; since the parser knows about artifact mode and about both files, it can see what probes are in the Scalars.yaml file which are not in the original file.
- the parser generates a JSON file that gets stored somewhere useful; the JSON file mirrors the definitions of the new scalars in a browser-digestible way (since yaml isn't browser friendly);
- when Telemetry starts, on developer builds, the browser looks for the JSON file and tries to load it;
- it registers the probes as dynamic scalars like done by Georg's patch.

This will work behind the scenes: devs will just need to change the same file all the docs already talk about. The try server will still offer test coverage because it will build without artifact mode.

@chutten, @georg what's your take on this?
Flags: needinfo?(gfritzsche)
Flags: needinfo?(chutten)
I like it except for its process aggregation semantics. Dynamic scalars aggregate to a single "dynamic" process. "Static" scalars aggregate to their accumulating process.

Generally this isn't too big a deal for JS-only probes since they tend to be main-process-only or content-process-only.

--------

Oh, and since they'll be dynamic, they won't show up in the same place in the ping payload if you build artefact vs building full. Which means they'll not show in the same place in about:telemetry if you build artefact vs building full.

I think this might be a sticking point for me. We may need to develop a dev-build-only way of dynamically-registering "static" scalars to the correct processes.
Flags: needinfo?(chutten)
(In reply to Chris H-C :chutten from comment #13)
> I like it except for its process aggregation semantics. Dynamic scalars
> aggregate to a single "dynamic" process. "Static" scalars aggregate to their
> accumulating process.

Yes, ideally this would be solved as Georg did in his patch, by providing a way to register "dynamic built-in scalars". This would make them show the as if they were from a "normal" build.

> I think this might be a sticking point for me. We may need to develop a
> dev-build-only way of dynamically-registering "static" scalars to the
> correct processes.

Yes, I should have expanded that a bit more in comment 12, sorry. We would still require a mechanism like that, as shown in the attached patch, to support the whole thing. I can't think of a better way :-\
Flags: needinfo?(gfritzsche)
Priority: P2 → P1
Assignee: nobody → alessio.placitelli
(In reply to Alessio Placitelli [:Dexter] from comment #12)
> Building up on the comments above, I have a technical proposal to try to
> solve this.
> 
> - when in "artifact" mode, in addition to the other pre-built files, we also
> should also ship a copy of the original "Scalars.yaml" file somewhere in the
> object directory (or where all the artifacts are stored);
> - when building in "artifact" mode we parse both the original (in the object
> dir) and the one in toolkit/components/telemetry; since the parser knows
> about artifact mode and about both files, it can see what probes are in the
> Scalars.yaml file which are not in the original file.

After discussing this with Georg, turns out we might do without shipping the "Scalars.yaml" file using artifacts. We can simply add all the definitions from the current version of the Scalars.yaml to a JSON file and simply ignore the errors coming from trying to register the scalars that are already defined.
See Also: → 1206117
Comment on attachment 8943684 [details]
Bug 1425909 - Enable adding scalars in artifact builds without rebuilding Firefox.

https://reviewboard.mozilla.org/r/214070/#review219858

::: commit-message-f095d:7
(Diff revision 1)
> +
> +This patch enables generating a JSON file that mirrors the scalar definitions
> +in Scalars.yaml. On local developer builds, this file is loaded when Firefox
> +starts to register all the scalars. If some change was introduced in the
> +definition files, the new scalar will be dynamically added.
> +The JSON definition file will be regenaret every time an artifact build

regenerated

::: toolkit/components/telemetry/ScalarInfo.h:36
(Diff revision 1)
>      : kind(aKind)
>      , dataset(aDataset)
>      , record_in_processes(aRecordInProcess)
>      , keyed(aKeyed)
> +    , builtin(aBuiltin)
>    {}

...do we want to do to the effort of #ifdef MOZILLA_OFFICIAL around all the storage and logic for dynamic builtins?
Comment on attachment 8943684 [details]
Bug 1425909 - Enable adding scalars in artifact builds without rebuilding Firefox.

https://reviewboard.mozilla.org/r/214070/#review219952

::: toolkit/components/telemetry/ScalarInfo.h:36
(Diff revision 1)
>      : kind(aKind)
>      , dataset(aDataset)
>      , record_in_processes(aRecordInProcess)
>      , keyed(aKeyed)
> +    , builtin(aBuiltin)
>    {}

Good point! I'm not sure though if that's helpful for supporting the "artifact" build system: we download pre-built stuff and I'm not sure how that would affect it. Let's leave this issue opened and check back with a build-peer when we get closer to review time.
Comment on attachment 8943684 [details]
Bug 1425909 - Enable adding scalars in artifact builds without rebuilding Firefox.

https://reviewboard.mozilla.org/r/214070/#review219952

> Good point! I'm not sure though if that's helpful for supporting the "artifact" build system: we download pre-built stuff and I'm not sure how that would affect it. Let's leave this issue opened and check back with a build-peer when we get closer to review time.

Oooh, you're very right about that. We'll need to build artefacts that support builtins. Fun.
Comment on attachment 8943684 [details]
Bug 1425909 - Enable adding scalars in artifact builds without rebuilding Firefox.

https://reviewboard.mozilla.org/r/214070/#review220240
Attachment #8943684 - Flags: review?(chutten)
Comment on attachment 8943684 [details]
Bug 1425909 - Enable adding scalars in artifact builds without rebuilding Firefox.

https://reviewboard.mozilla.org/r/214070/#review220290


Static analysis found 12 defects in this patch.
 - 3 defects found by clang-tidy
 - 9 defects found by mozlint

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/telemetry/TelemetryController.jsm:1086
(Diff revision 3)
> +
> +    // Load the file off the disk.
> +    let scalarJSProbes = {};
> +    try {
> +      let fileContent = await OS.File.read(scalarProbeFile.path, { encoding: "utf-8" });
> +      scalarJSProbes = JSON.parse(fileContent, (property, value) => {

Error: Expected to return a value at the end of arrow function. [eslint: consistent-return]

::: toolkit/components/telemetry/TelemetryController.jsm:1096
(Diff revision 3)
> +        }
> +
> +        switch (value) {
> +          case "nsITelemetry::SCALAR_TYPE_COUNT":
> +            return Telemetry.SCALAR_TYPE_COUNT;
> +            break;

Error: Unreachable code. [eslint: no-unreachable]

::: toolkit/components/telemetry/TelemetryController.jsm:1099
(Diff revision 3)
> +          case "nsITelemetry::SCALAR_TYPE_COUNT":
> +            return Telemetry.SCALAR_TYPE_COUNT;
> +            break;
> +          case "nsITelemetry::SCALAR_TYPE_BOOLEAN":
> +            return Telemetry.SCALAR_TYPE_BOOLEAN;
> +            break;

Error: Unreachable code. [eslint: no-unreachable]

::: toolkit/components/telemetry/TelemetryController.jsm:1102
(Diff revision 3)
> +          case "nsITelemetry::SCALAR_TYPE_BOOLEAN":
> +            return Telemetry.SCALAR_TYPE_BOOLEAN;
> +            break;
> +          case "nsITelemetry::SCALAR_TYPE_STRING":
> +            return Telemetry.SCALAR_TYPE_STRING;
> +            break;

Error: Unreachable code. [eslint: no-unreachable]

::: toolkit/components/telemetry/TelemetryScalar.cpp:316
(Diff revision 3)
>  class ScalarUnsigned : public ScalarBase
>  {
>  public:
>    using ScalarBase::SetValue;
>  
> -  ScalarUnsigned() : mStorage(0) {};
> +  ScalarUnsigned(bool builtin = false) : ScalarBase(builtin), mStorage(0) {};

Error: Bad implicit conversion constructor for 'scalarunsigned' [clang-tidy: mozilla-implicit-constructor]

::: toolkit/components/telemetry/TelemetryScalar.cpp:456
(Diff revision 3)
>  class ScalarString : public ScalarBase
>  {
>  public:
>    using ScalarBase::SetValue;
>  
> -  ScalarString() : mStorage(EmptyString()) {};
> +  ScalarString(bool builtin = false) : ScalarBase(builtin), mStorage(EmptyString()) {};

Error: Bad implicit conversion constructor for 'scalarstring' [clang-tidy: mozilla-implicit-constructor]

::: toolkit/components/telemetry/TelemetryScalar.cpp:537
(Diff revision 3)
>  class ScalarBoolean : public ScalarBase
>  {
>  public:
>    using ScalarBase::SetValue;
>  
> -  ScalarBoolean() : mStorage(false) {};
> +  ScalarBoolean(bool builtin = false) : ScalarBase(builtin), mStorage(false) {};

Error: Bad implicit conversion constructor for 'scalarboolean' [clang-tidy: mozilla-implicit-constructor]

::: toolkit/components/telemetry/gen_scalar_data.py:111
(Diff revision 3)
> +
> +        if category not in scalar_definitions:
> +            scalar_definitions[category] = OrderedDict()
> +
> +        scalar_definitions[category][scalar.name] = OrderedDict({
> +            "kind": scalar.nsITelemetry_kind,

Error: Remove bad quotes. [flake8: Q000]

::: toolkit/components/telemetry/gen_scalar_data.py:112
(Diff revision 3)
> +        if category not in scalar_definitions:
> +            scalar_definitions[category] = OrderedDict()
> +
> +        scalar_definitions[category][scalar.name] = OrderedDict({
> +            "kind": scalar.nsITelemetry_kind,
> +            "keyed": scalar.keyed,

Error: Remove bad quotes. [flake8: Q000]

::: toolkit/components/telemetry/gen_scalar_data.py:113
(Diff revision 3)
> +            scalar_definitions[category] = OrderedDict()
> +
> +        scalar_definitions[category][scalar.name] = OrderedDict({
> +            "kind": scalar.nsITelemetry_kind,
> +            "keyed": scalar.keyed,
> +            "record_on_release": True if scalar.dataset == "opt-out" else False,

Error: Remove bad quotes. [flake8: Q000]

::: toolkit/components/telemetry/gen_scalar_data.py:113
(Diff revision 3)
> +            scalar_definitions[category] = OrderedDict()
> +
> +        scalar_definitions[category][scalar.name] = OrderedDict({
> +            "kind": scalar.nsITelemetry_kind,
> +            "keyed": scalar.keyed,
> +            "record_on_release": True if scalar.dataset == "opt-out" else False,

Error: Remove bad quotes. [flake8: Q000]

::: toolkit/components/telemetry/gen_scalar_data.py:115
(Diff revision 3)
> +        scalar_definitions[category][scalar.name] = OrderedDict({
> +            "kind": scalar.nsITelemetry_kind,
> +            "keyed": scalar.keyed,
> +            "record_on_release": True if scalar.dataset == "opt-out" else False,
> +            # TODO: use buildinfo to expire probes at build time.
> +            "expired": False,

Error: Remove bad quotes. [flake8: Q000]
Georg, here's the ni? as discussed over IRC The interesting part lives in TelemetryScalar.cpp, in the |internal_GetScalarByEnum| and |CreateSnapshot| (plus their keyed variants). In order to let dynamic scalars show up as "builtin", the most convenient path forward was to add them to the per-process storage already in place. This is done by adding them to the same storage, right after the last "builtin" scalar (e.g. at index |ScalarCount|).

Thoughts?
Flags: needinfo?(gfritzsche)
Comment on attachment 8943684 [details]
Bug 1425909 - Enable adding scalars in artifact builds without rebuilding Firefox.

https://reviewboard.mozilla.org/r/214070/#review220398

::: toolkit/components/telemetry/TelemetryScalar.cpp:1138
(Diff revision 3)
> +    // Since this is a dynamic builtin scalar, fixup its id. We need this to store
> +    // the scalar data in the real process storage, instead of the "dynamic" one. For
> +    // this to work, the index used to store the scalar must start after the last
> +    // static scalar (i.e. ScalarCount).
> +    fixupScalarId = aId.id + static_cast<uint32_t>(mozilla::Telemetry::ScalarID::ScalarCount);

This is a short-cut to avoid refactoring and possibly a bit fragile.
Can we find better solutions, like
- putting them in a separate storage container
- making the storage key be a composite of {id, builtin}
- something else?
Flags: needinfo?(gfritzsche)
Attachment #8943684 - Flags: review?(chutten)
Comment on attachment 8943684 [details]
Bug 1425909 - Enable adding scalars in artifact builds without rebuilding Firefox.

https://reviewboard.mozilla.org/r/214070/#review220398

> This is a short-cut to avoid refactoring and possibly a bit fragile.
> Can we find better solutions, like
> - putting them in a separate storage container
> - making the storage key be a composite of {id, builtin}
> - something else?

Good point. Took me a bit, but I decided to switch to a separate storage container. Doesn't look too bad now!
Comment on attachment 8943684 [details]
Bug 1425909 - Enable adding scalars in artifact builds without rebuilding Firefox.

For some reason MozReview doesn't automatically pick up the :build-peer as a reviewer. Manually flagging :)

Please kindly review the changes to the moz.build file. The idea is to run the referenced python script every time a build is performed, regardless of the mode: it should work for artifact and "build faster" builds as well.

There are a few open questions:

- Is there any way to conditionally run the python script only on artifact and "build faster" builds?
- We want this feature to work only on local developer builds (MOZILLA_OFFICIAL not being defined). If we conditionally enable our C++ changes based on that, would it still do the right thing on artifact builds?
Attachment #8943684 - Flags: review?(core-build-config-reviews)
Comment on attachment 8943684 [details]
Bug 1425909 - Enable adding scalars in artifact builds without rebuilding Firefox.

https://reviewboard.mozilla.org/r/214070/#review221364

Just some small stuff, I think.

::: toolkit/components/telemetry/Telemetry.cpp:1738
(Diff revision 4)
> +NS_IMETHODIMP
> +TelemetryImpl::RegisterBuiltinScalars(const nsACString& aCategoryName,
> +                                      JS::Handle<JS::Value> aScalarData,
> +                                      JSContext* cx)
> +{
> +  return TelemetryScalar::RegisterScalars(aCategoryName, aScalarData, true, cx);

Should we have some sort of parent-process-only-protection here. Like:

if (!XRE_IsParentProcess()) { return NS_OK; }

::: toolkit/components/telemetry/TelemetryScalar.cpp:2163
(Diff revision 4)
>      StaticMutexAutoLock locker(gTelemetryScalarsMutex);
> -    // Iterate the scalars in gScalarStorageMap. The storage may contain empty or yet to be
> +
> +    // The snapshotting function is the same for both static and dynamic builtin scalars.
> +    // We can use the same function and store the scalars in the same output storage.
> +    auto snapshotter = [aDataset, &locker, &scalarsToReflect]
> +                       (ProcessesScalarsMapType& aProcessStorage, bool aIsBuiltinDynamic)->nsresult

Isn't there supposed to be whitespace around the "->"?

::: toolkit/components/telemetry/TelemetryScalar.cpp:2291
(Diff revision 4)
>    {
>      StaticMutexAutoLock locker(gTelemetryScalarsMutex);
> -    // Iterate the scalars in gKeyedScalarStorageMap. The storage may contain empty or yet
> +
> +    auto snapshotter = [aDataset, &locker, &scalarsToReflect]
> +                       (ProcessesKeyedScalarsMapType& aProcessStorage,
> +                        bool aIsBuiltinDynamic)->nsresult

Ditto whitespace

::: toolkit/components/telemetry/TelemetryScalar.cpp:2570
(Diff revision 4)
> +    for (auto childIter = scalarStorage->Iter(); !childIter.Done(); childIter.Next()) {
> +      ScalarBase* scalar = static_cast<ScalarBase*>(childIter.Data());
> +      n += scalar->SizeOfIncludingThis(aMallocSizeOf);
> +    }
> +  }
> +  // Also account for keyed scalar data coming from parent and child processes.

copy-pasta. This comment should say something about dynamic builtins.

::: toolkit/components/telemetry/TelemetryScalar.cpp:2578
(Diff revision 4)
> +      static_cast<KeyedScalarStorageMapType*>(iter.Data());
> +    for (auto childIter = scalarStorage->Iter(); !childIter.Done(); childIter.Next()) {
> +      KeyedScalar* scalar = static_cast<KeyedScalar*>(childIter.Data());
> +      n += scalar->SizeOfIncludingThis(aMallocSizeOf);
> +    }
> +  }

These four sequential for loops, mostly identical, make my eyes twitch. I doubt it would help, but I can't help thinking that it'd be cleaner with some sort of templating/lambda magic. Not an issue, just a case of "we wouldn't have this if we'd written it in Rust"

::: toolkit/components/telemetry/gen_scalar_data.py:99
(Diff revision 4)
> +def generate_JSON_definitions(output, *filenames):
> +    """ Write the scalar definitions to a JSON file.
> +
> +    :param output: the file to write the content to.
> +    :param filenames: a list of filenames provided by the build system.
> +           We only support a sngle file.

*single

::: toolkit/components/telemetry/tests/unit/test_TelemetryScalars_buildFaster.js:93
(Diff revision 4)
> +  // Write the scalar definition to the spec file in the binary directory.
> +  let definitionFile = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> +  definitionFile = Services.dirsvc.get("GreBinD", Ci.nsIFile);
> +  definitionFile.append("ScalarArtifactDefinitions.json");
> +
> +  await CommonUtils.writeJSON(DYNAMIC_SCALAR_SPEC, definitionFile.path);

Do we have to have every test perform file I/O? There's definitely benefit in testing that valid and invalid files are correctly parsed from disk... but for testing the behaviour of the dynamic builtins (that they're aggregated and reported like builtins) we shouldn't need to write to disk, right

::: toolkit/components/telemetry/tests/unit/test_TelemetryScalars_buildFaster.js:120
(Diff revision 4)
> +               TEST_SCALAR1 + " must have the correct value.");
> +
> +  // Clean up.
> +  await TelemetryController.testShutdown();
> +  await OS.File.remove(definitionFile.path);
> +});

Speaking of which, we should probably test an invalid file.
Attachment #8943684 - Flags: review?(chutten) → review-
Comment on attachment 8943684 [details]
Bug 1425909 - Enable adding scalars in artifact builds without rebuilding Firefox.

https://reviewboard.mozilla.org/r/214070/#review221364

> Should we have some sort of parent-process-only-protection here. Like:
> 
> if (!XRE_IsParentProcess()) { return NS_OK; }

That's already taken care of in [TelemetryScalar.cpp](https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/toolkit/components/telemetry/TelemetryScalar.cpp#2338-2340)

> These four sequential for loops, mostly identical, make my eyes twitch. I doubt it would help, but I can't help thinking that it'd be cleaner with some sort of templating/lambda magic. Not an issue, just a case of "we wouldn't have this if we'd written it in Rust"

Good point! Done!

> Do we have to have every test perform file I/O? There's definitely benefit in testing that valid and invalid files are correctly parsed from disk... but for testing the behaviour of the dynamic builtins (that they're aggregated and reported like builtins) we shouldn't need to write to disk, right

Good point.  I'd still like to keep at least one test loading the file: we need to make sure that the definitions are loaded from the correct path (the directory where the executable lives). But I'll change the other test.
Comment on attachment 8943684 [details]
Bug 1425909 - Enable adding scalars in artifact builds without rebuilding Firefox.

See comment 28 for the original request.
Attachment #8943684 - Flags: review?(core-build-config-reviews)
Comment on attachment 8943684 [details]
Bug 1425909 - Enable adding scalars in artifact builds without rebuilding Firefox.

https://reviewboard.mozilla.org/r/214070/#review221694
Attachment #8943684 - Flags: review?(chutten) → review+
ni? me

- mention that we support scalars in firefox-sourcedocs
- update the MDN page
Flags: needinfo?(alessio.placitelli)
Comment on attachment 8943684 [details]
Bug 1425909 - Enable adding scalars in artifact builds without rebuilding Firefox.

https://reviewboard.mozilla.org/r/214070/#review222348

r=me on the moz.build changes.
Attachment #8943684 - Flags: review+
Attachment #8943684 - Flags: review?(core-build-config-reviews) → review+
Comment on attachment 8946707 [details]
Bug 1425909 - Mention the support for scalars in artifact and build-faster workflows.

https://reviewboard.mozilla.org/r/216678/#review222454

The MDN doc update is still pending?
Attachment #8946707 - Flags: review?(chutten) → review+
(In reply to Chris H-C :chutten from comment #37)
> Comment on attachment 8946707 [details]
> Bug 1425909 - Mention the support for scalars in artifact and build-faster
> workflows.
> 
> https://reviewboard.mozilla.org/r/216678/#review222454
> 
> The MDN doc update is still pending?

Yup, I will leave my ni? pending and update the page once this lands.
Ok, I had to deal with a weird Android-only outage and some bitrot. Should be all set now!
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0b5d96b317c3
Enable adding scalars in artifact builds without rebuilding Firefox. r=chutten,froydnj
https://hg.mozilla.org/integration/autoland/rev/6bcaae60c82e
Mention the support for scalars in artifact and build-faster workflows. r=chutten
Backed out for mochitest C3 failures on test_memoryReporters.xul.

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=159896651&repo=autoland&lineNumber=3099
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/569ae07d9dd8
Backed out 2 changesets for mochitest C3 failures on test_memoryReporters.xul. on a CLOSED TREE
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a5f5b76a8bd6
Enable adding scalars in artifact builds without rebuilding Firefox. r=chutten,froydnj
https://hg.mozilla.org/integration/autoland/rev/eda22ad91a29
Mention the support for scalars in artifact and build-faster workflows. r=chutten
https://hg.mozilla.org/mozilla-central/rev/a5f5b76a8bd6
https://hg.mozilla.org/mozilla-central/rev/eda22ad91a29
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(In reply to Alessio Placitelli [:Dexter] from comment #34)
> - update the MDN page

I updated the "Restrictions" section at: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds#Restrictions
Flags: needinfo?(alessio.placitelli)
(In reply to Alessio Placitelli [:Dexter] from comment #53)
> (In reply to Alessio Placitelli [:Dexter] from comment #34)
> > - update the MDN page
> 
> I updated the "Restrictions" section at:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Build_Instructions/Artifact_builds#Restrictions

I saw your commit -- thanks for keeping the Artifacts documentation fresh!
(In reply to Nick Alexander :nalexander from comment #54)
> (In reply to Alessio Placitelli [:Dexter] from comment #53)
> > (In reply to Alessio Placitelli [:Dexter] from comment #34)
> > > - update the MDN page
> > 
> > I updated the "Restrictions" section at:
> > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> > Build_Instructions/Artifact_builds#Restrictions
> 
> I saw your commit -- thanks for keeping the Artifacts documentation fresh!

Cheers, you're welcome! ;)
See Also: → 1448945
You need to log in before you can comment on or make changes to this bug.