[RDY] Add ":profile dump" and ":profile stop" #2427

mhinz · 2015-04-14T15:35:56Z

Currently the logfile (":profile start {logfile}") only gets written when Vim
exits. This new command allows to dump the log immediately without exiting.

splinterofchaos · 2015-04-14T16:05:28Z

How do you make sure the profile doesn't get dumped twice?

ZyX-I · 2015-04-14T16:09:43Z

@splinterofchaos Why do you think this is needed? profile dump dumps intermediate (current) state, by the time NeoVim exits there may and probably will be some changes. Even if there are none (e.g. when using profile pause before profile dump) there is not much harm in overwriting file with the results with identical contents.

mhinz · 2015-04-14T16:37:48Z

At exit it will be overwritten again. dump is mainly meant to get access to the log without restarting Vim.

I guess I could also add :profile stop while being at it. Yes/no?

splinterofchaos · 2015-04-14T16:42:12Z

there is not much harm in overwriting file with the results with identical contents.

Good point.

I guess I could also add :profile stop while being at it. Yes/no?

Maybe it's not as necessary as I thought, but if we want to give the user finer control over when to dump the profiling info, I think it also makes sense to let the user specify whether we should or not.

ZyX-I · 2015-04-15T05:42:11Z

src/nvim/ex_cmds2.c

+  } else if (STRCMP(eap->arg, "stop") == 0) {
+    profile_dump();
+    do_profiling = PROF_NONE;
+    set_vim_var_nr(VV_PROFILING, 0L);


I am wondering what will happen if you do profile start after profile stop? It should start new profiling, but AFAIR neither of profile_dump, set_vim_var is clearing the associated structures.

You're right. I'll fix that.

~~profile_fname also needs to be freed.~~

mhinz · 2015-04-16T11:22:22Z

I added static void profile_reset(void) for proper cleanup on calling :profile stop.

splinterofchaos · 2015-04-16T13:19:49Z

src/nvim/ex_cmds2.c

+  scriptitem_T *si;
+
+  for (int id = 1; id <= script_items.ga_len; id++) {
+    si = &SCRIPT_ITEM(id);


si should be declared here

splinterofchaos · 2015-04-16T13:35:38Z

LGTM 👍

mhinz · 2015-04-16T18:40:43Z

The :profile stop command now resets all profiling information for sourced files and functions.

oni-link · 2015-04-16T19:14:16Z

src/nvim/eval.h

@@ -3,6 +3,47 @@

 #include "nvim/profile.h"

+// All user-defined functions are found in this hashtable.
+hashtab_T func_hashtab;


Should be a declaration, like extern hashtab_T func_hashtab; and only be defined in one file (eval.c?).

Not sure if this is something we're changing, but the current convention I see is to use EXTERN (defined in nvim/globals.h), which initializes it when included from main.c.

mhinz · 2015-04-17T00:45:12Z

Uhm, what's the proper fix of:

/home/travis/build/neovim/neovim/src/nvim/eval.h:42:16: error: ‘dumuf’ defined but not used [-Werror=unused-variable]
cc1: all warnings being treated as errors

Just making it non-static is probably the wrong approach. And I can't just use (void)dumuf in the file scope. And just throwing that in a random function seems wrong, too.

Maybe there's is another way where we don't have to use that dummy variable in the first place?

splinterofchaos · 2015-04-17T03:03:57Z

Uhm, what's the proper fix of:

That's what extern does, but extern requires there be exactly one source file that actually defines the value (or just declares it without extern). That's what the macro EXTERN defines in globals.h does. (When included from main.c, there won't be an extern on the declaration.)

mhinz · 2015-04-17T08:13:48Z

@splinterofchaos I guess I was a bit tired already, since I didn't see that it has to be extern unless the dummy variable was created in the macro itself. EXTERN is neat.

mhinz · 2015-04-17T12:03:27Z

[...]
05:39:48,221 WARN  - /home/quickbuild/buildagent/workspace/root/neovim/pull-requests-automated/src/nvim/eval.c: In function ‘call_user_func’:
05:39:48,221 WARN  - /home/quickbuild/buildagent/workspace/root/neovim/pull-requests-automated/src/nvim/eval.c:19078:16: error: ‘call_start’ may be used uninitialized in this function [-Werror=uninitialized]
05:39:48,455 WARN  - cc1: all warnings being treated as errors
05:39:48,458 WARN  - make[3]: *** [src/nvim/CMakeFiles/nvim.dir/eval.c.o] Error 1
05:39:48,459 WARN  - make[2]: *** [src/nvim/CMakeFiles/nvim.dir/all] Error 2
05:39:48,459 WARN  - make[1]: *** [src/nvim/CMakeFiles/nvim.dir/rule] Error 2
05:39:48,459 WARN  - make: *** [nvim] Error 2
05:39:48,562 INFO  - Executing post-execute action...
05:39:48,562 ERROR - Step 'master>buildall>build-node?testNode=linux-64>build-and-run-tests>build-and-run-tests-parameterized?buildType=Release>configure-neovim-and-build-nvim' is failed: Failed to run command: mkdir -p build/Release && cd build/Release && cmake -G "Unix Makefiles" -DBUSTED_OUTPUT_TYPE=junit -DCMAKE_BUILD_TYPE=Release -DTRAVIS_CI_BUILD=ON ../.. && make nvim nvim-test unittest-headers tty-test
Command return code: 2
Command error output: /home/quickbuild/buildagent/workspace/root/neovim/pull-requests-automated/src/nvim/eval.c: In function ‘call_user_func’:
/home/quickbuild/buildagent/workspace/root/neovim/pull-requests-automated/src/nvim/eval.c:19078:16: error: ‘call_start’ may be used uninitialized in this function [-Werror=uninitialized]
cc1: all warnings being treated as errors
make[3]: *** [src/nvim/CMakeFiles/nvim.dir/eval.c.o] Error 1
make[2]: *** [src/nvim/CMakeFiles/nvim.dir/all] Error 2
make[1]: *** [src/nvim/CMakeFiles/nvim.dir/rule] Error 2
make: *** [nvim] Error 2
[...]

Why would http://neovim-qb.szakmeister.net/build/581 fail for me (this branch doesn't touch these functions) but not for other PRs?

@jszakmeister ^

jszakmeister · 2015-04-17T13:20:33Z

There might be a couple of explanations. First, I recently changed the build steps to enable errors on warnings (just like the Travis build). The difference here is that the QB instance is also doing a full release build (without asserts) which may expose some things the Travis build (currently) does not. #2353 fixed several, but it appears there are a couple more. They either slid enable before I got things enabled, or I just missed them somehow (which is entirely possible--though I was pretty certain I ran a build everywhere to make sure it was clean before turning it on). So, I'd say ignore the error for this PR. Hopefully the remaining warnings can be fixed in short order (though someone else might have to step and do it since I'm at a conference for the next few days).

mhinz · 2015-04-17T14:23:07Z

Okeydokey, thanks for the explanation. (And have fun at the conference!)

jszakmeister · 2015-04-24T08:30:33Z

BTW, the call_start bit should be fixed on master now. You'll need to rebase to bring it into your branch.

This adds two new tests for: :profile dump :profile stop

Currently the logfile (":profile start {logfile}") only gets written when Vim exits. This new command allows to dump the log immediately without exiting.

This writes the logfile and stops profiling.

- shada/msgpack editor plugin #3270 - VimL Dict notifications #3603 - Note: API for this feature may change. - :profile dump, :profile stop #2427 - :oldfiles! #3611 - TermOpen, TermClose events #3653 - fix: shada/viminfo: Do not save unlisted and quickfix buffers #3581

marvim added the RFC label Apr 14, 2015

mhinz force-pushed the add-profile-dump branch from c6854a5 to c5683a2 Compare April 14, 2015 15:45

mhinz changed the title ~~[RFC] Add ":profile dump"~~ [WIP] Add ":profile dump" Apr 14, 2015

marvim added WIP and removed RFC labels Apr 14, 2015

mhinz force-pushed the add-profile-dump branch from c5683a2 to d24745c Compare April 15, 2015 00:16

mhinz changed the title ~~[WIP] Add ":profile dump"~~ [RFC] Add ":profile dump" Apr 15, 2015

marvim added RFC and removed WIP labels Apr 15, 2015

ZyX-I reviewed Apr 15, 2015
View reviewed changes

justinmk added the enhancement label Apr 15, 2015

mhinz force-pushed the add-profile-dump branch 2 times, most recently from 143115f to 04e855e Compare April 16, 2015 11:19

splinterofchaos reviewed Apr 16, 2015
View reviewed changes

mhinz force-pushed the add-profile-dump branch from 04e855e to 74a4181 Compare April 16, 2015 13:24

mhinz force-pushed the add-profile-dump branch 2 times, most recently from 07514a8 to 96303c3 Compare April 16, 2015 18:39

oni-link reviewed Apr 16, 2015
View reviewed changes

mhinz force-pushed the add-profile-dump branch 2 times, most recently from d0d10d9 to e17d195 Compare April 17, 2015 00:01

mhinz force-pushed the add-profile-dump branch from d52d4ab to eb29a63 Compare April 17, 2015 00:47

mhinz force-pushed the add-profile-dump branch from eb29a63 to 1eafad4 Compare April 17, 2015 08:12

mhinz force-pushed the add-profile-dump branch 2 times, most recently from 079952c to dc6868e Compare April 17, 2015 09:32

mhinz mentioned this pull request Apr 17, 2015

[RFC] Initialize call_start to prevent warnings #2444

Closed

mhinz changed the title ~~[RFC] Add ":profile dump"~~ [RFC] Add ":profile dump" and ":profile stop" Apr 17, 2015

mhinz force-pushed the add-profile-dump branch from dc6868e to 1ce98cf Compare April 24, 2015 08:56

mhinz force-pushed the add-profile-dump branch 3 times, most recently from 9c42b2e to f31f29d Compare November 8, 2015 13:55

mhinz changed the title ~~[RFC] Add ":profile dump" and ":profile stop"~~ [RDY] Add ":profile dump" and ":profile stop" Nov 8, 2015

marvim added RDY and removed RFC labels Nov 8, 2015

mhinz added 4 commits November 10, 2015 02:49

Remove unused constants

e4d9176

Add test/functional/ex_cmds/profile_spec.lua

6908511

This adds two new tests for: :profile dump :profile stop

Add ":profile dump"

588bf76

Currently the logfile (":profile start {logfile}") only gets written when Vim exits. This new command allows to dump the log immediately without exiting.

Add ":profile stop"

1902ee5

This writes the logfile and stops profiling.

mhinz force-pushed the add-profile-dump branch from f31f29d to 1902ee5 Compare November 10, 2015 01:49

mhinz merged commit 1902ee5 into neovim:master Nov 10, 2015

jszakmeister removed the RDY label Nov 10, 2015

mhinz added a commit that referenced this pull request Nov 10, 2015

Merge PR #2427 'Add ":profile dump" and ":profile stop"'

971e5c2

mhinz deleted the add-profile-dump branch November 11, 2015 16:24

Uh oh!

[RDY] Add ":profile dump" and ":profile stop" #2427

[RDY] Add ":profile dump" and ":profile stop" #2427

Uh oh!

Conversation

mhinz commented Apr 14, 2015

Uh oh!

splinterofchaos commented Apr 14, 2015

Uh oh!

ZyX-I commented Apr 14, 2015

Uh oh!

mhinz commented Apr 14, 2015

Uh oh!

splinterofchaos commented Apr 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhinz commented Apr 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

splinterofchaos commented Apr 16, 2015

Uh oh!

mhinz commented Apr 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhinz commented Apr 17, 2015

Uh oh!

splinterofchaos commented Apr 17, 2015

Uh oh!

mhinz commented Apr 17, 2015

Uh oh!

mhinz commented Apr 17, 2015

Uh oh!

jszakmeister commented Apr 17, 2015

Uh oh!

mhinz commented Apr 17, 2015

Uh oh!

jszakmeister commented Apr 24, 2015

Uh oh!

Uh oh!