It's been a persistent frustration that "anonymous runnable" stubbornly clings to #5 or #6 position when runnables are ranked by frequency. A while ago I wrote a clang transformation to label as many runnables as possible. Given that we're still having trouble with labeling, I think it's time to land it. Nathan, what sort of process do we usually follow for large patches like this? Can I get one person to review it, or do I need to ask for review on a per-module basis?

(In reply to Bill McCloskey (:billm) from comment #0) > It's been a persistent frustration that "anonymous runnable" stubbornly > clings to #5 or #6 position when runnables are ranked by frequency. A while > ago I wrote a clang transformation to label as many runnables as possible. > Given that we're still having trouble with labeling, I think it's time to > land it. Oh wow. That looks great! > Nathan, what sort of process do we usually follow for large patches like > this? Can I get one person to review it, or do I need to ask for review on a > per-module basis? I think the unwritten rule is that if your patch is an essentially mechanical patch (e.g. adding an obvious argument, sed-like renames, API refactoring etc.), a single review is OK. If your patch starts wandering into someplace more complicated, or someplace where module knowledge might be required, then separate patches are a good idea. Separate patches are OK in any event, of course, but a single patch is of course easier to manage.

This is the tool I used to generate most of the names. It works as follows: 1. The name it generates is the fully qualified name, but with no template parameters, and with "mozilla::" stripped off if it's there. 2. It labels anything of the form NS_NewRunnableFunction, New{,NonOwning}{,Cancelable}RunnableMethod. The name of the function passed in is used if possible. Otherwise it uses the name of the function calling NewRunnable (this is the case for lambdas, for example). 3. Any class that inherits from Runnable is changed so that its constructor passes a name to the Runnable constructor. I tried my best to use "Runnable(...)" instead of "mozilla::Runnable(...)". It's not perfect though: if you're in a "using mozilla;" scope, it will still use "mozilla::Runnable". I think this could be hard to do correctly because of unified compilation. 4. Calls to InitWithFuncCallback on timers are changed to pass the name of the callback.

This is a product of both manual and automatic labeling. I had to resort to manual work for the platform-dependent stuff. I tried to fix up the formatting by running clang-format on the diff that was output. This sometimes produces not-great results since the Mozilla clang style doesn't exactly match our actual style (or at least my interpretation of it). I think it's the best we can do without a ton of incredibly boring effort, though.

This patch deletes the Runnable constructor that doesn't take a name. This way we can't regress.

This patch removes the NewRunnableFoo variants that don't take a name parameter. It prevents regressions of this form. It also removes one dimension from our boilerplate.

I'm not sure why I originally tried to delegate to Runnable::GetName in this method. It doesn't make any sense. It also caused us to return the wrong value once SchedulerGroup started passing a non-empty name to the Runnable constructor.

Let me make sure I understand the rationale here: - We have telemetry (or other statistics) on what runnables are most frequent. - Some of the most frequent runnables are unnamed and therefore lumped together in said statistics. - This uncertainty causes issues with trying to prioritize how runnables are tagged for the scheduler: we obviously want to tag the most frequent runnables first, so we get the biggest win possible. - Therefore, we're going to NAME ALL THE RUNNABLES so our statistics are as complete as possible. - We also want to ensure that people name all runnables in the future, so we don't wind up with a top-ten <anonymous runnable> again. A couple of thoughts, particular to the last point: - WDYT about adopting something like the FROM_HERE macro: http://dxr.mozilla.org/mozilla-central/source/security/sandbox/chromium/base/location.h#99 that's used in conjunction with PostTask: http://dxr.mozilla.org/mozilla-central/source/security/sandbox/chromium/base/task_runner.h#125 so that people don't have to think super-hard about what name to use? I don't know what a good default would be, since we'd have to make it a string, but maybe something like __FILE__ ## __func__ would work? (We don't want __LINE__, as that'd probably shift around too much.) I guess people already come up with names for Mutex() and so forth where necessary, so maybe this isn't as big of a deal as I think it is. - Alternatively, could we just use said macro in the automatic labeling run? - Do you have any statistics on what the size increase from this is? Say, if you run `size libxul.so` before and after this change, what are the numbers? The numbers would probably increase a lot more with the __FILE__ ## __func__ proposed above, though =/

Comment on attachment 8878250 [details] [diff] [review] fix up SchedulerGroup::GetName Review of attachment 8878250 [details] [diff] [review]: ----------------------------------------------------------------- Is it possible that this is causing problems for the names that we see already? Hm, I guess not, it's just extra checking overhead currently.

Comment on attachment 8878248 [details] [diff] [review] delete nameless NewRunnable variants Review of attachment 8878248 [details] [diff] [review]: ----------------------------------------------------------------- Fewer NewRunnable* functions is definitely a good thing.

(In reply to Nathan Froyd [:froydnj] from comment #7) > Let me make sure I understand the rationale here: This is correct. > A couple of thoughts, particular to the last point: > > - WDYT about adopting something like the FROM_HERE macro: I'm not a big fan of FROM_HERE. In C++, __func__ doesn't include the class name or anything, so it's often going to be a pretty generic name. Chrome also includes the filename and line number, but as you note, the line number will mess up telemetry as things move around. I guess the filename and function name would be okay, but definitely less user friendly, and bigger. Especially since __FILE__ will probably be something like "/c/builder/source/gecko/dom/base/nsFrameMessageManager.cpp". I guess we could prettify that at runtime though. The thing I really like about including the actual string is that it's very greppable. You just enter the string in DXR/Searchfox (with quotes around it) and it takes you directly to the correct spot. Another issue with FROM_HERE is that it doesn't give you the name you want for NewRunnableMethod. It tells you the name of the function that called NewRunnableMethod, not the name of the method. That seems pretty confusing to me. The same is true for timer initialization. It is kind of annoying to have to come up with a name, but it does mean we'll get better names. > - Alternatively, could we just use said macro in the automatic labeling run? I'm not sure I understand the difference between this and the FROM_HERE proposal. > - Do you have any statistics on what the size increase from this is? Say, > if you run `size libxul.so` before and after this change, what are the > numbers? The numbers would probably increase a lot more with the __FILE__ > ## __func__ proposed above, though =/ Let me check once I get into an office (faster builds). Keep in mind that the runnable name is not saved in RELEASE_OR_BETA builds, so I think we shouldn't pay any cost for them.

(In reply to Bill McCloskey (:billm) from comment #10) > (In reply to Nathan Froyd [:froydnj] from comment #7) > > A couple of thoughts, particular to the last point: > > > > - WDYT about adopting something like the FROM_HERE macro: > > Another issue with FROM_HERE is that it doesn't give you the name you want > for NewRunnableMethod. It tells you the name of the function that called > NewRunnableMethod, not the name of the method. That seems pretty confusing > to me. The same is true for timer initialization. This is a good point, and I think a dealbreaker for doing a FROM_HERE-esque scheme. > > - Alternatively, could we just use said macro in the automatic labeling run? > > I'm not sure I understand the difference between this and the FROM_HERE > proposal. The proposal was to simply have the tool that you wrote insert FROM_HERE instead of some made-up name. > > - Do you have any statistics on what the size increase from this is? Say, > > if you run `size libxul.so` before and after this change, what are the > > numbers? The numbers would probably increase a lot more with the __FILE__ > > ## __func__ proposed above, though =/ > > Let me check once I get into an office (faster builds). Keep in mind that > the runnable name is not saved in RELEASE_OR_BETA builds, so I think we > shouldn't pay any cost for them. I think this is the case too, assuming Runnable's constructor is appropriately inlined. But we keep tabs on how big e.g. Fennec's libxul is, so if we know the increase is going to be big, we can at least point at some analysis and say that we knew about it beforehand and it shouldn't be a problem on release.

Comment on attachment 8878245 [details] [diff] [review] patch Review of attachment 8878245 [details] [diff] [review]: ----------------------------------------------------------------- I checked out representative examples in various files, but I did not read the whole patch.

Regarding the size issue, the difference is pretty small. I did an opt build with profiling enabled and debug/debug symbols disabled. The size of libxul.so was 142MB both with and without my patches here. This kinda makes sense. The naming patch has around 1300 hunks, so probably about 1300 additional strings. Each one takes maybe 10 or 20 bytes, so the total increase is at most 30KB. I also did a build to simulate the RELEASE_OR_BETA case. When I run |strings| on libxul, the names passed to the Runnable constructor are not present. I did find a problem with the names passed to NewRunnableMethod/Function--they get included regardless. To fix this, I added an #ifdef to SetRunnableName. With that fix (which I'll fold into the NewRunnable patch, I don't think it needs review), I don't see any runnable names in libxul. The timer names still get included since they have to go through XPIDL, but there aren't very many of those so it's probably okay. I'll land this tonight when the tree is quieter.

Pushed by wmccloskey@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/301e80f20046 Change SchedulerGroup::GetName so it doesn't delegate to mozilla::Runnable (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/9846de3bd954 Provide names for all runnables in the tree (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/ac1e2098e59e Delete default Runnable ctor so name must be provided (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/4d33d0e50338 Remove NewRunnable functions that lack a name param (r=froydnj)

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=108437696&repo=mozilla-inbound

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ff9c1e9bf931 Backed out changeset 4d33d0e50338 https://hg.mozilla.org/integration/mozilla-inbound/rev/2f1200166be8 Backed out changeset ac1e2098e59e https://hg.mozilla.org/integration/mozilla-inbound/rev/e61d71c2a3a3 Backed out changeset 9846de3bd954 https://hg.mozilla.org/integration/mozilla-inbound/rev/0f30b67d296a Backed out changeset 301e80f20046 for bustage

Pushed by wmccloskey@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ce7c27a7a91e Change SchedulerGroup::GetName so it doesn't delegate to mozilla::Runnable (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/4f6302a98ae4 Provide names for all runnables in the tree (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/328c356c0bcd Delete default Runnable ctor so name must be provided (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/f4521f4c0fc3 Remove NewRunnable functions that lack a name param (r=froydnj)

sorry had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=108833970&repo=mozilla-inbound - i guess jya is working on this also in Bug 1374596

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/45438f565c49 Backed out changeset f4521f4c0fc3 https://hg.mozilla.org/integration/mozilla-inbound/rev/17bfc89021ae Backed out changeset 328c356c0bcd https://hg.mozilla.org/integration/mozilla-inbound/rev/4472d2623ece Backed out changeset 4f6302a98ae4 https://hg.mozilla.org/integration/mozilla-inbound/rev/af979c2baf41 Backed out changeset ce7c27a7a91e for bustage in fatal error C1128: number of sections exceeded object file format limit: compile with /bigobj

Pushed by wmccloskey@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/df2c5bb77d90 Change SchedulerGroup::GetName so it doesn't delegate to mozilla::Runnable (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/312f7a5a2c08 Provide names for all runnables in the tree (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/09dfd61cf247 Delete default Runnable ctor so name must be provided (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/682a3a2d7f4f Remove NewRunnable functions that lack a name param (r=froydnj)

https://hg.mozilla.org/mozilla-central/rev/df2c5bb77d90 https://hg.mozilla.org/mozilla-central/rev/312f7a5a2c08 https://hg.mozilla.org/mozilla-central/rev/09dfd61cf247 https://hg.mozilla.org/mozilla-central/rev/682a3a2d7f4f