Eliminate runtime reflection in Operators#27
Conversation
|
What is the rationale for this change? Does it improve performance? It is a net increase of 42 lines of code, and makes the codebase less DRY. Is it just because people consider reflection to be ugly or ill-advised? If the issue is that repeated calls to |
|
See also related discussion at #24 (comment), which explains that this change does improve performance. But if we cache the calculated-via-reflection list, I feel like that should achieve the vast majority of the performance gain. (Is |
|
When we can shift anything from runtime to compile time, we should. Reflection can be made to hit less. But, it will always be an unnecessary hit. Eliminating reflection from the main code makes the project easier to understand imho. When you see the list, you know what is going on. The test code reflection will remind the programmer if they added an operator but did not add it to the list. GraalVM may or may not work as is with the reflection. I know it will work 100% with the list approach. How often has the list changed over the years? |
8387aac to
883ce78
Compare
…atorsTest instead.
We can grow the list as each operator is constructed.
883ce78 to
0d5ae61
Compare
|
@JaredDavis22 I found a way of doing what you want here, without explicitly replicating the list of operators. Please take a look and let me know what you think. If you like it, I'll merge this. |
|
Looks great. Thanks. Do we still need reflection in OperatorsTest with this solution? |
Nah, I think we're OK without it! |
Eliminate reflection during runtime. A test is added to ensure the manually maintained list in Operators remains in sync with the declared Operator entries.