Re: Refactoring proposal for the observer service
On 04/01/2018 23:47, Gijs Kruitbosch wrote: On 04/01/2018 22:49, Gabriele Svelto wrote: On 04/01/18 00:05, Gijs Kruitbosch wrote: Unfortunately, there are quite a lot ( https://searchfox.org/mozilla-central/search?q=obs.addObserver&case=false®exp=false&path= -- sync, the add-ons manager, session store, etc. etc.). That's not exactly what I meant. JS clients are fine as long as the topic has already been declared. The issue with artifact builds arises only when adding an entirely new topic that is used only in JS code (i.e. both the notifier and listener are JS-only). I don't know how many patches we've had but my guess is that they're not that many. JS code usually has better ways to deal with this kind of scenarios than using the observer service. But the examples I gave are exactly that. sync, the add-ons manager and sessionstore all live in JS-land and have, to the best of my knowledge, (almost?) no C++ consumers of their "own" observer notifications. And they all have several observer notifications. They are also not the only components that do this, just the first few I spotted in the searchfox query. Though of course, if we wanted to, we could decide that JS-only observer topics should get their own mechanism/component entirely, which also gets rid of this problem and avoids any xpconnect costs for this stuff, at the cost of having 2 implementations publish/subscribe... But hey, it's just a pub/sub implementation, not rocket science - and esp. in JS-land I know the limitations of the observer service as-is in terms of parameters etc. has been unfortunate at times. ~ Gijs ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Refactoring proposal for the observer service
On 04/01/2018 22:49, Gabriele Svelto wrote: On 04/01/18 00:05, Gijs Kruitbosch wrote: Unfortunately, there are quite a lot ( https://searchfox.org/mozilla-central/search?q=obs.addObserver&case=false®exp=false&path= -- sync, the add-ons manager, session store, etc. etc.). That's not exactly what I meant. JS clients are fine as long as the topic has already been declared. The issue with artifact builds arises only when adding an entirely new topic that is used only in JS code (i.e. both the notifier and listener are JS-only). I don't know how many patches we've had but my guess is that they're not that many. JS code usually has better ways to deal with this kind of scenarios than using the observer service. But the examples I gave are exactly that. sync, the add-ons manager and sessionstore all live in JS-land and have, to the best of my knowledge, (almost?) no C++ consumers of their "own" observer notifications. And they all have several observer notifications. They are also not the only components that do this, just the first few I spotted in the searchfox query. Ideally, I would like a solution where especially JS-only observer topics don't require a non-artifact build. I've given this a little thought and there could be a simpler solution, though a little hacky. Instead of making the number of topic a compilation constant one could put it somewhere were it could be discovered at runtime (such as a preference) then leave a few empty slots in the table for "unexpected" observers. In an artifact build one would add the new topic which would regenerate the JS code, update the number of topics but not the C++ side. At runtime C++ code would then see the "unexpected" topic but since it would still end up in the expected range at runtime would handle it correctly. It wouldn't be super-pretty but it would work for common cases such as adding only one new topic (or a few). Well, I assume we ideally want the file to be organized. Not willy-nilly collections of "useful observer topics", like the way in which all.js and Histograms.json are basically a giant mess. I would further assume that we (automatically) number items sequentially. So adding items in the middle of the list and not recompiling the C++ is going to cause off-by-N errors for however many N observers you add, right? I don't see a good way of distinguishing the "new" topics at build-time and numbering them differently. Of course, patches could add the topic at the end for testing and then move it for review/landing, but that's still going to be annoying for developers, esp. if there's multiple review cycles, and we'll probably forget to do it sometime, etc. etc. So not very ergonomic... ~ Gijs ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Refactoring proposal for the observer service
On 04/01/18 22:47, Nathan Froyd wrote: > This is very doable, it just requires some build system hackery: we > accept preprocessed/generated WebIDL files, and generated IDL files > would require basically the same approach. I can help with the build > system hackery if you want to continue pursuing this approach. If it's possible then I think it would be worth pursuing. With this approach the constants would be held together with the methods within the interface object/class versus three separate entities of my current approach (the interface object/class, a C++ enum and a JS object). Having everything in one place is definitely a better approach from a code design perspective (and would prevent potential issues such as including the interface definition but not the header with the enum declaration). We'd also retain the other advantages such as the ability to add further metadata to the declaration, having the constants generated instead of written by hand, etc... The only drawback I can think of is that we'd lose the ability to use an enum in C++ for extra type safety. But that really isn't a big deal. For crash annotations it mattered because the JS interface is built on top of an already exposed C++ one which could be made type-safe. For observers the interface used is always the generated one which - being the same for JS and C++ - cannot use C++ enums directly anyway. Gabriele signature.asc Description: OpenPGP digital signature ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Refactoring proposal for the observer service
On Thu, Jan 4, 2018 at 4:44 PM, Gabriele Svelto wrote: > On 04/01/18 22:39, Ben Kelly wrote: >> Or make your "generator" >> create the idl which then creates the js/c++? > > I tried as that could have worked! Unfortunately it doesn't seem to be > possible ATM. mach bailed out with a weird error when I tried to put an > IDL file among the generated ones. I didn't really dig into it but I > suspect that since we already generate code from IDL files they're not > expected to be generated in turn. This is very doable, it just requires some build system hackery: we accept preprocessed/generated WebIDL files, and generated IDL files would require basically the same approach. I can help with the build system hackery if you want to continue pursuing this approach. -Nathan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Refactoring proposal for the observer service
On 04/01/18 22:39, Ben Kelly wrote: > We can't have a comment explaining "please add any new constants in > sequential order in the following list"? We could, but what about removing one? Also if we have hundreds (or thousands) the risk that one is accidentally duplicated is significant. My rationale for this is that managing topics the way we do know simply doesn't scale so if we can avoid manual work we should. > Or make your "generator" > create the idl which then creates the js/c++? I tried as that could have worked! Unfortunately it doesn't seem to be possible ATM. mach bailed out with a weird error when I tried to put an IDL file among the generated ones. I didn't really dig into it but I suspect that since we already generate code from IDL files they're not expected to be generated in turn. Gabriele signature.asc Description: OpenPGP digital signature ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Refactoring proposal for the observer service
On Thu, Jan 4, 2018 at 4:35 PM, Gabriele Svelto wrote: > On 03/01/18 23:30, Ben Kelly wrote: > > Could we use our existing idl/webidl/ipdl for this? It would be nice > > not to have to maintain another code generator in the tree if possible. > > > AFAIK there is no way in IDL to declare an enum. Constants can be > declared but one needs to manually assign them a value which makes it > unsuitable for this task. Also there's no way to attach other metadata > to the declaration (such as a mandatory comment). > We can't have a comment explaining "please add any new constants in sequential order in the following list"? Or make your "generator" create the idl which then creates the js/c++? ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Refactoring proposal for the observer service
On 03/01/18 23:30, Ben Kelly wrote: > Could we use our existing idl/webidl/ipdl for this? It would be nice > not to have to maintain another code generator in the tree if possible. AFAIK there is no way in IDL to declare an enum. Constants can be declared but one needs to manually assign them a value which makes it unsuitable for this task. Also there's no way to attach other metadata to the declaration (such as a mandatory comment). Don't get me wrong though, I am convinced it would be better if this could be done in IDL because it could then be attached to the relevant interface (such as nsIObserverService) and we wouldn't need separate generation of the C++ and JS code, but we would need to significantly extend it for the purpose. Gabriele signature.asc Description: OpenPGP digital signature ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Refactoring proposal for the observer service
On 2018-01-04 10:00 AM, Nathan Froyd wrote: … 2) How would one shoehorn this into *DL? The options that come to mind are: - Separate methods for every observer topic, which sounds terrible from a code duplication perspective. Though maybe this would be nice for JS clients, so we could say things like: Services.obs.notifyXPCOMShutdown(...) which would save on some xpconnect traffic and representing a large-ish enum in JS? - WebIDL enums (I think), which would carry a large space penalty and make everything that wants to use the observer service depend on code from dom/, which seems undesirable. - IDL enums, which aren't reflected into JS, so we'd need some custom work there. As Ben said, XPIDL const's on an interface seem like a missing option that would work with JS. Random example: https://dxr.mozilla.org/mozilla-central/search?q=RECOVER_SESSION&redirect=false - IPDL doesn't even have the concept of definable enums, and wouldn't reflect things into JS, so we'd need even more work there. (Some of this may be desirable; we talked about extending IPDL bits into JS in Austin, and kmaglione felt this was reasonable, so...) The first and third don't sound *too* bad, but I don't think that writing a one-off code generator would be strictly worse... MattN ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Refactoring proposal for the observer service
On Thu, Jan 4, 2018 at 10:00 AM, Nathan Froyd wrote: > On Wed, Jan 3, 2018 at 5:30 PM, Ben Kelly wrote: > > On Wed, Jan 3, 2018 at 5:09 PM, Gabriele Svelto > wrote: > >> 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): > > > > Could we use our existing idl/webidl/ipdl for this? It would be nice > not to > > have to maintain another code generator in the tree if possible. > > I don't understand this objection on two levels: > It wasn't an objection. It was a question. If possible, can we use something we already have? The answer can be "no". > 1) Why does maintaining another code generator in tree hurt anything? > We have many one-off code generators for specific purposes: > > https://searchfox.org/mozilla-central/search?q=GENERATED_ > FILES&case=false®exp=false&path=moz.build > > Some of these use non-Python generators (e.g. the a11y files and gfx > shaders), but there are probably enough to count on multiple hands. > > I would expect modifications of the code generator to be infrequent, > and the code generator itself is liable to be a screenful or so of > straightforward code. > Sure, but having someone understand and able to modify additional code generators does require additional effort. Its not impossible, but it makes it harder. So it would be nice to avoid if we can. If we can't avoid it, then ok. > - WebIDL enums (I think), which would carry a large space penalty and > make everything that wants to use the observer service depend on code > from dom/, which seems undesirable. > - IDL enums, which aren't reflected into JS, so we'd need some custom > work there. > Yea, I was mostly thinking webidl or idl enums. The enums could be stuck on an idl interface as constants, no? Anyway, I was mostly just asking that we consider these options before writing something new. If they're not a good fit, then so be it. Thanks. Ben ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Refactoring proposal for the observer service
On Wed, Jan 3, 2018 at 5:30 PM, Ben Kelly wrote: > On Wed, Jan 3, 2018 at 5:09 PM, Gabriele Svelto wrote: >> 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): > > Could we use our existing idl/webidl/ipdl for this? It would be nice not to > have to maintain another code generator in the tree if possible. I don't understand this objection on two levels: 1) Why does maintaining another code generator in tree hurt anything? We have many one-off code generators for specific purposes: https://searchfox.org/mozilla-central/search?q=GENERATED_FILES&case=false®exp=false&path=moz.build Some of these use non-Python generators (e.g. the a11y files and gfx shaders), but there are probably enough to count on multiple hands. I would expect modifications of the code generator to be infrequent, and the code generator itself is liable to be a screenful or so of straightforward code. 2) How would one shoehorn this into *DL? The options that come to mind are: - Separate methods for every observer topic, which sounds terrible from a code duplication perspective. Though maybe this would be nice for JS clients, so we could say things like: Services.obs.notifyXPCOMShutdown(...) which would save on some xpconnect traffic and representing a large-ish enum in JS? - WebIDL enums (I think), which would carry a large space penalty and make everything that wants to use the observer service depend on code from dom/, which seems undesirable. - IDL enums, which aren't reflected into JS, so we'd need some custom work there. - IPDL doesn't even have the concept of definable enums, and wouldn't reflect things into JS, so we'd need even more work there. (Some of this may be desirable; we talked about extending IPDL bits into JS in Austin, and kmaglione felt this was reasonable, so...) The first and third don't sound *too* bad, but I don't think that writing a one-off code generator would be strictly worse... -Nathan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Refactoring proposal for the observer service
On 03/01/2018 23:18, Nihanth Subramanya wrote: ++ from me as well, this sounds awesome. Ideally, I would like a solution where especially JS-only observer topics don't require a non-artifact build. A JS forwarder for the "real" observer service may help with that goal, by making the "new" arguments into strings transparently, and using the same string as a webidl name property for artifact builds. This would mean "new" JS-only observer calls in an artifact build would be slower than the int-based "real" ones (and would perform "normally" once you actually did a "real" build, ditto for distributed stuff). I think that's an OK trade-off. Would you mind expanding on how you see this working? Specifically, I didn’t understand "using the same string as a webidl name property for artifact builds”. We'd have a JS artifact-build-only version of the topics list that was a proxy on the 'real' thing. So you'd access ObsTopics.Foo, and if the 'real' thing knew about "Foo", it'd just forward the relevant int. If not, it would return the topic string (so "Foo"). When the JS forwarder for the 'real' service gets called with a string (instead of an int), it notifies/adds/removes observers on itself only, and doesn't pass the relevant call to C++. The reason for using strings and forwarding is that I don't see a reasonable way for the build system to distinguish topics that the binary blobs it's using know about, from topics they have no idea about (that the developer just added to the manifest). Whereas at runtime it's easy to make that determination. I guess you could maybe do it at build time by firing up xpcshell as part of the build process, and then generate new ints for the new topics into the JS file, but that seems cumbersome at best. Plus you'd probably want to make the observer service throw for calls with invalid topic ints, so then you need a way to distinguish the new JS-only ones on the JS side before passing it to the 'real' observer service. I guess we could have the C++ side expose the "maximum valid topic int" or whatever, and the JS side could check that... but having them be different JS types seems simpler. Also related: in this JS forwarder world you’re describing, what exactly does the workflow look like for a frontend developer using artifact builds? I’d imagine the following: Get an artifact build ready to go Make changes, including adding the new observer topics to the manifest (or list, or whatever you want to call it) mach build faster Changes are immediately testable, though there will be a perf cost unless you do a “normal” build. Yes, that seems right to me. ~ Gijs ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Refactoring proposal for the observer service
On 01/04/2018 12:30 AM, Ben Kelly wrote: On Wed, Jan 3, 2018 at 5:09 PM, Gabriele Svelto wrote: 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): Could we use our existing idl/webidl/ipdl for this? It would be nice not to have to maintain another code generator in the tree if possible. Yeah, I was thinking this too. I really wish we don't add yet another code generator. idl or webidl should be ok. But I see artifact builds being rather big issue here(, even though I don't personally use them). Building FF has become so very slow, that avoiding extra build steps whenever possible should be a goal. We should try to make our platform easier to approach, not harder. -Olli ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Refactoring proposal for the observer service
This is really interesting, thanks for looking at this! On 03/01/2018 22:09, Gabriele Svelto wrote: This would have quite a few coding benefits: - It would make it far easier to retire/rename a topic, since ... JS would throw. Why would it? I think it would pass undefined (as the topic wouldn't exist on the "ObserverTopics" object) which I expect would get coerced by xpconnect to just '0'. Unless we either use a Proxy for the JS implementation that throws when you access an unknown property (which would be runtime-expensive) or unless WebIDL provides a way to do this that avoids the coercion when passing it to an observer service method, and instead throws from that method if it gets passed `undefined` for a topic. It would also help with code size, performance and memory use: So, one thing that I have been thinking about in relation to https://bugzilla.mozilla.org/show_bug.cgi?id=1425466 is that the observer service when invoking lots of JS observers currently does (something like) this: - get called from a single place (in JS/C++, doesn't matter much); - get list of observers - call each observer with the same arguments. For JS observers, this means crossing from C++ to JS, and back with exceptions or whatever for cases where there are "many" observers for a single topic (x-ref https://bugzilla.mozilla.org/show_bug.cgi?id=1195386 ), this is going to be inefficient because we go back and forth across the xpconnect boundary N times for N observers, every time getting JS values/wrappers for the subject and data, etc. etc. This would be better if there was a JS side to things that handled all the dispatching to JS observers, and would "forward" exactly 1 observer to the 'real' observer service. I'm not sure if the costs justify optimizing this. I haven't had time to profile this or see how often this happens if you don't run a browser session with hundreds or thousands of tabs (and tbh, most if not all the consumers which add that many observers ought to be optimized to Not Do That anyway). > - 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. Unfortunately, there are quite a lot ( https://searchfox.org/mozilla-central/search?q=obs.addObserver&case=false®exp=false&path= -- sync, the add-ons manager, session store, etc. etc.). We run into this problem with other things as well - telemetry being one that currently does something like what you propose (so we can't change it in an artifact build and that's pretty annoying) and I know Nicholas Nethercote (CC'd) has been looking at making the prefs behave similarly. Ideally, I would like a solution where especially JS-only observer topics don't require a non-artifact build. A JS forwarder for the "real" observer service may help with that goal, by making the "new" arguments into strings transparently, and using the same string as a webidl name property for artifact builds. This would mean "new" JS-only observer calls in an artifact build would be slower than the int-based "real" ones (and would perform "normally" once you actually did a "real" build, ditto for distributed stuff). I think that's an OK trade-off. Put differently, if we end up in a place where touching any prefs, telemetry or observers means frontend folks can't use artifact builds, that'll have significant negative impact on our development speed, as well as the ability of new contributors to build and contribute code to Firefox. ~ Gijs ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Refactoring proposal for the observer service
On Wed, Jan 3, 2018 at 5:09 PM, Gabriele Svelto wrote: > 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): > Could we use our existing idl/webidl/ipdl for this? It would be nice not to have to maintain another code generator in the tree if possible. Otherwise seems like a good idea. Thanks. Ben ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Refactoring proposal for the observer service
On Wed, Jan 3, 2018 at 5:09 PM, Gabriele Svelto wrote: > - 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. +1. Not having a place to "declare" / document observer messages has been one of the things bothering me about the observer service for a long time. Cheers, Botond ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Refactoring proposal for the observer service
That would be great! On 03/01/18 23:09, Gabriele Svelto wrote: > 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. > ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Refactoring proposal for the observer service
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
