[PATCH] profiler2: can't be disabled in game and other suggestions

[fabio]

When pressing F11 HTTP profiler get enabled on port 8000 along with on-screen profiler. Pressing F11 again more time disable the on-screen profiler but not the HTTP one. This is its main option:

hotkey.profile2.enable = "F11"              ; Enable HTTP/GPU modes for new profiler

I'd suggest these:

• change the key for HTTP profiler (Alt+F11 or Ctrl+F11 ?) so that one can enable the on-screen profiler without the HTTP one;
• be able to disable the HTTP profiler pressing a second time the hotkey;
• print a message on screen when enabling/disabling it (or always shows a message when enabled);
• eventually open a web browser pointing to the profiler web page when enabling.

[historic_bruno]

What if you want the HTTP profiler enabled but not the in-game profiler display? Using another hotkey for that seems messy and takes up a valuable hotkey we could use for something else. We could always opt for a config setting, as with the new JS debugger, or some combination of hotkey + config setting.

[fabio]

It make sense.

[fabio]

Thinking at it again a good choice could be Alt+F11 or Ctrl+F11:

• F11 and Shift+F11 are already used for the profiler, so another F11-combination will be for profiler2;
• doesn't abuse a dedicated hotkey;
• a key combination make it unlikely to press it by error.

I tested it and work as expected, eventually Alt+F11 is better.

[historic_bruno]

Ctrl+F11 sounds good to me, with a warning/message notifying that the profiling server has been enabled. If disabling can be done cleanly, then that would be OK too, I suppose.

Replying to fabio:

• eventually open a web browser pointing to the profiler web page when enabling.

I don't think this is necessarily a good idea, for the same reason I mentioned on the forum (about the JS debugger). Releases don't typically bundle the contents of source/tools and the exact path could change, but it adds unnecessary clutter to work around that. If releases bundled the debugger and profiler scripts, it would be different, otherwise I think we should just zip them up and offer them as a separate download. It's not hard to open the HTML file in your browser of choice and bookmark it ;)

[metalhead]

Please review my patch.

Overview:

• F11 toggles only GPU profiler
• Ctrl+F11 toggles profiler2 HTTP server mode (starts or stops mongoose http server)
• listening port is specified in config
• show LOGERROR with some details if HTTP server fails to start instead of ENSURE

[leper]

Please read and follow SubmittingPatches when doing so. (Especially the part about adding the review keyword as otherwise the ticket will not show up in the review query and the patch will get lost.)

[historic_bruno]

Thanks for the patch. Here are some comments from a quick review:

• The GPU profiler is closely coupled to the HTTP profiler, it doesn't make sense to have a separate hotkey for it. They should be enabled and disabled together. In that case the config option might become hotkey.profile2.toggle
• profiler2.listen_port is misleading because it's not only a port, but an IP as well. It should either be renamed to indicate a listening "interface" or whatever the best term is, or changed so it really is only a port e.g. 8000. The port alone seems less confusing and less error prone, yet should work in almost every case.
• It looks wrong to have a const char* that you're later assigning to.
• The config value is read in an unconventional way, typically we set the fallback/default first, then use CFG_GET_VAL to read the value from the config system, if it exists, like so:

  std::string listeningPort = "127.0.0.1:8000";
  CFG_GET_VAL("profiler2.listen_port", String, listeningPort);
  

• We tend to not use underscores, either in variable names or config entries, see Coding_Conventions for more conventions.
• The Mongoose error handling belongs in a separate patch on a new ticket.

[giannis]

Please review my patch. The patch is based on previous work by metalhead and other suggestions from the comments

Overview:

• GPU profiler and HTTP server are now enabled and shut down together.
• Ctrl+F11 to toggle both GPU mode and HTTP server
• F11 still works for the in- game profiler
• added messages in terminal when a part of the profiler is enabled/disabled.

[giannis]

based on the previous patch and suggestions

[Itms]

Hi kingbasil, and thanks for that patch.

Your patch contains a lot of unnecessary line endings, tabs, etc. changes, that make it difficult to review. I don't know if it comes from your diff tool or if you corrected this manually, but if you want to submit style fixes you should create another patch.

From a functional point of view, you should correct your enable/disable messages. They are really intrusive on Windows platforms, and the debug_DisplayMessage doesn't suit IMO. I recommend you use LOGMESSAGERENDERER (see wiki:Logging) to inform the user of the changes. Also, "Attention" is not everyday English, "Warning" or "Information" would be better.

Thanks for working on this!

[giannis]

Thanks for reviewing my patch

The new patch I attached fixes the problems you described:

• replaced the messages using LOGMESSAGERENDERER
• ignored white spaces in diff

I look forward to helping you guys. This game is very cool

[Itms]

Hi kingbasil, and sorry for having put this patch review aside for a shameful amount of time.

There are still style issues (a bracket with wrong indentation, you use spaces while you should use tabs, and you didn't update the copyright dates in the source files you changed), so I suggest you check wiki:Coding_Conventions before submitting the patch.

The code itself seems OK, the profiler seems to work as expected but when I test, I always get twice every message about systems turned on or off. Could you investigate this?

Thanks in advance, I hope this long review time didn't discourage you from helping out there. If you want to make us do things (like reviewing faster :P), or discuss about your patch, don't hesitate to drop by the dev IRC channel (​#0ad-dev on QuakeNet)!

[Stan]

Hopefully Fix KingBasil's patch following coding conventions.

[Stan]

I think you get twice the message cause it's called in the main function and in the subfunction. Not sure though.

[Itms]

In 15723:

Some tweaks to profiler2:

• separate the HTTP profiling server from the on-screen profiler
• allow shutting down the HTTP profiler
• print messages when enabling/disabling HTTP and GPU profilers

Patch by kingbasil, fixes #1862

[Itms]

I corrected the style fixes and couldn't reproduce the double messages I got last time. I checked the code to be sure and I think there is no problem with the functionality, that must be me. :)

Thanks for that patch!