Repository navigation
Conversation
|
|
76fce04 to
1cbf57b
Compare
|
Agreed with @pevik, the first 3 changes are no problem. I'd like to review the next 3 in a little more depth. I'm a bit skeptical that they'll improve things in the way they're intended to, so I want a little more time to either convince myself that they're good or come up with convincing evidence that they don't... |
1cbf57b to
14e165e
Compare
Signed-off-by: Sami Kerola <[email protected]>
Reference: https://lgtm.com/projects/g/iputils/iputils/?mode=list Signed-off-by: Sami Kerola <[email protected]>
Convert int types to long before multiplicatio. Notice that ntokens that is used to store the value is already long. Reference: https://lgtm.com/projects/g/iputils/iputils/?mode=list Signed-off-by: Sami Kerola <[email protected]>
The cmsgbuf is 4096 bytes of stack, that is wasted when pinging IPv4 addresses. The rcvd_tbl is always needed, but 8192 is enough stack size that it is better to use proper allocation function so that possible allocation failure can be detected. Secondly making the whole of run time struct to fit into a single memory page should maximize probability of it to be in cache. Signed-off-by: Sami Kerola <[email protected]>
Inlining functions is more often cache bloat than performance gain. While checking where ping is most expensive pr_addr() turned out stall the most, and with a simple stack variable scoping that function is lighter. Signed-off-by: Sami Kerola <[email protected]>
Signed-off-by: Sami Kerola <[email protected]>
arping.c:692:33: warning: ISO C forbids empty initializer braces [-Wpedantic] ping.c:1620:43: warning: ISO C forbids empty initializer braces [-Wpedantic] Signed-off-by: Sami Kerola <[email protected]>
14e165e to
68803cb
Compare
|
Hi @kerolasa. I had hoped to provide something more definitive to express my skeptecism about the removal of the inline keywords and the use of heap space for memory, but I haven't been able to devote the time to do so. So please feel free to merge. A couple of thoughts on the topic: When running with any optimization flags, as is done in the iputils build configuration, gcc doesn't actually inline any functions marked with the inline keyword anyway. When any For very small functions, inlining is almost certainly going to result in more efficient code. That's even more the case when the function is only called once within a given C file, as is the case, for example with the Regarding the use of |
|
Hello @nmeyerhans and @pevik. Let me merge the first three changes, and abandon the rest. What comes inlining sometimes you win, other times not. http://www.cs.technion.ac.il/users/yechiel/c++-faq/inline-and-perf.html is pretty good short explanation how complicated this stuff is. My understanding benefit of having functions in cache is often better than inlining them. Anyway this is not so important that I want to push removal hard. Reason why I want to malloc() rather than use stack is pretty simple. I want to fit the runtime configuration to a single memory page so that it is very effectively cached. As a minor thing catching allocation failure is possible with malloc(), while stack allocations are more nasty but since we are talking about couple hundred bytes this is pretty theoretical note. |
|
@kerolasa IMHO commit "arping / ping: fix couple warnings" is also obviously correct (backwards compatibility). Feel free to merge it with my ack. |
|
I dared to push your fix |
3 participants
Couple commits I wrote while back that have sit in my local branch. I though to combine these with ping timing work, but that change has become so big that it's better to get small commits reviewed separately. Also, the ping time stuff is taking lots of time to get done (pun intended), and being busy with that I could concurrently wait feedback.