Minor code and style improvements in ui/repl.c #9732
Conversation
- use array variables instead of pointers - add const where possible - use enums to assign values for high getopt options insteaf of possibly error-prone manual assignment. Also documents the code better
- use array variables instead of pointers - add const where possible - use enums to assign values for high getopt options insteaf of possibly error-prone manual assignment. Also documents the code better
|
Thanks! Welcome to julia; I know I (and probably many of our contributors and users) have benefited from your excellent articles and open source work over the years. |
|
Yes, in particular I found https://www.akkadia.org/drepper/tls.pdf very helpful when implementing TLS support for the JIT (still ongoing work). |
|
Indeed; I'm a fan of "what every programmer should know about memory." Thanks. |
jl_egal is an exported and recursively called function which puts some constraints on the optimizer. The way jl_egal is currently structured it pretty much as all the code needed for the comparisons inlined, including the more complex parts to compare tuples and fields. The code goes to great length to optimize special cases which can be treated simpler but the generalization of the rest of the code causes problems. Specifically, the more complex parts of the comparison process require more registers for the optimized code to use. The results in the function starting with a large prologue which saves all the callee-save registers that are used, followed in the end by the respective epilogue. This means that even though in some cases the function will almost immediately return because of the special case handling, all the work for of the prologue and epilogue still has to be done. I ran limited profiling (mostly the arrayops test case) and the jl_egal function shows up on position 3 with 5.48%. With the simple change in this patch this is reduced to 4.64%. The patch is trivial, no real code changes. To prevent the complex prologue/epilogue from unconditionally bing created the complex code blocks are moved into their own functions and then these functions are called. To prevent the optimizer from negating this work the functions must be marked appropriately. Fortunately I've found that this is already done in some cases elsewhere in the code base so I'm sure the change will not create any problem.
|
Likewise. Welcome! |
|
I've pushed one more patch, this time a performance optimization. The somewhat lengthy commit message should explain this. If these types of patches are not appropriate let me know. |
|
lgtm. Maybe the noinline code could be put into a macro like the one we have for Line 124 in 4e65472 |
|
Could you take the text from the commit message and turn it into a comment near the implementation of |
- introduce macros NOINLINE and NOINLINE_DECL - adjust existing code in task.c - rewrite change for jl_egal using NOINLINE - add extensive comment explaining jl_egal change
|
I've added macros NOINLINE and NOINLINE_DECL (the latter being necessary due to the way function declarations are handled in gcc vs msft compilers) and I added the comment. What I forgot in the previous comments: I saw a speedup of around 4% due to this change. |
Minor code and style improvements in ui/repl.c
|
This looks great, many thanks! What were you timing when you saw that 4% speedup? |
|
I just ran some of the tests, mostly arrayops. |
|
Looks like it was arrayops: 89f3b58 |
possibly error-prone manual assignment. Also documents the
code better