TL;DR this is a proposal to refactor the observer service to use a machine-generated list of integers for the topics (disguised as enums/JS constants) instead of arbitrary strings.
Long version:
I've been working on bug 1348273 [1] in an attempt to gather all our
crash annotations into a machine-readable list that could be used to
generate a list of (named) constants. Specifically an enumeration for
C++ and a frozen object holding constant fields for JS. These constants
would then be used instead of the strings when invoking the crash
annotation interface.
As I was hacking this stuff together it occurred to me that the observer
service has the same kind of interface and the same kind of issues:
figuring out what a particular topic is for is quite hard, and often
requires a fair amount of grep'ing and hg blame'ing to figure out where
it's coming from and what it does. There's no single place where it can
be documented. There's no way to tell what the subject is and what the
data string contains (and if it contains a string at all, I've seen
callers stuffing a pointer to a non-string object in there). Retiring or
renaming a topic is also a nightmare, as it's extremely hard to verify
that you've fixed all its uses.
So after validating my approach in that bug (which is almost ready) I've
thought that it might be time to give the observer service the same
treatment. First of all we'd have a list of topics (I've picked YAML for
the list but it could be anything that fits the bill):
QuitApplication:
description: Notification sent when the application is about to quit
ProfileBeforeChange:
description: Notification sent before the profile is lost
...
Which would generate a C++ enum:
// Generated enum
enum class ObserverTopic : uint32_t {
QuitApplication = 0,
ProfileBeforeChange = 1,
...
}
And a JS object:
const ObserverTopic = Object.freeze({
QuitApplication: 0,
ProfileBeforeChange: 1,
...
}
Callers would change so that they'd look like this:
os->NotifyObservers(nullptr, ObserverTopic::QuitApplication, nullptr);
Or this:
Services.obs.notifyObservers(null, ObserverTopic.QuitApplication);
Pretty straightforward stuff.
This would have quite a few coding benefits:
- It would make trivially simple to document the topics, their subjects
and payloads. Potentially this could even be integrated with our
generated documentation and code search tools making developer lives a
lot easier.
- It would make it far easier to retire/rename a topic, since C++ code
still using an unknown topic would fail to compile, and JS would throw.
- It would prevent typos from slipping in.
- It would make the naming consistent (compared to current things like
"apz:cancel-autoscroll", "APZ:FlushActiveCheckerboard" and
"apz-repaints-flushed").
It would also help with code size, performance and memory use:
- It makes the generated code a lot leaner, since the identifiers are
now integers under the hood, which require far less code to handle than
strings. For reference, on a Linux PGO build my patch for bug 1348273
shrinks libxul.so by ~35KiB while touching only ~100 call sites, the
observer service has thousands.
- We wouldn't need a hash-table anymore. A fixed-size array of topics,
each holding a vector of listeners, suffices and it can be indexed
directly. Besides the obvious performance improvements (no string
comparisons, no need to walk a sparse structure with all the cache/TLB
effects that come with it) it would also save memory.
While the above effects would be hard to measure (and possibly below the
noise threshold) they would definitely be there; and as we learned with
Quantum significant performance improvements in a codebase as large as
Firefox often come from plenty of small fixes.
This isn't without drawbacks though. The biggest ones I could think of
would be:
- We'd have a new header file that would be included in a lot of places,
and adding, removing or changing a topic would touch it, causing much
recompile.
- One would not be able to add/remove/change a topic in an artifact
build. I'm not sure how many JS-only users of the observer service there
are but if they're not many then this wouldn't be a big deal.
- This would most certainly need significant changes in Thunderbird too,
but I'd help with that, I promise.
So, what do you think? This is non-trivial work but I think that it
would be worth the fuss.
Gabriele
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1348273
Make AnnotateCrashReport() more robust by turning annotations into a
well known list of constants
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
