Some more small optimizations #3216
Conversation
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/3216/ to see the changes. Benchmarks is based on merge(s) with master |
|
Note that points for today's PRs are missing in the benchmarks linked above, so the comparison is misleading. You can look at it side-by-side with http://dotty-bench.epfl.ch/3214 instead. |
|
Interestingly this is the first time in a while that we have a code change that affects the speed of the empty class/object/file benchmarks, and in a good way too :). |
|
Morale: Java reflection is not only evil, it's also very slow, be careful! |
| private def hasRedefinedMethod(cls: Class[_], name: String): Boolean = { | ||
| val clsMethods = cls.getDeclaredMethods | ||
| if (cls.eq(classOf[TreeTransform]) || cls.eq(classOf[NoTreeTransform]) || |
There was a problem hiding this comment.
Not clear we need this anymore, because of ed5eb24.
We have just reduced the # of calls to this method by a factor 7.
There was a problem hiding this comment.
Yeah, it's more of a micro-optimization now. I can remove the added cache map if you think it's not worth the complexity now.
It turns out that this method takes time because getDeclaredMethods take time, so we avoid calling it when possible.
|
FYI @smarter Now the points for one PR accumulate, so it's possible to test multiple variations of the same PR continuously and see all the points in one graph. |
|
Cool! That's useful, but at the same time could be misleading if master changed since the last benchmarks for the PR, maybe the info bubble for the points should mention which commit from master was merged with which commit from the PR? |
|
The simplest solution is to just update the bot message to make it clear that which commit is merged with which master(hash). Does that address the concern? Note it's also possible to provide a command |
|
This all sounds like good ideas :). Instead of erasing previous points, I suggest using a different URL when retesting instead. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/3216/ to see the changes. Benchmarks is based on merge(s) with master |
|
@odersky Looks like the hasRedefinedMethod optimizations are still significant (in http://dotty-bench.epfl.ch/3216/ compare 6fd9569 to the previous point which is master, especially for empty file/object/class) |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/3216-2/ to see the changes. Benchmarks is based on merging with master (be01457) |
|
Github doesn't always pickup the |
|
test performance please Test for the PR liufengyun/bench#63 |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/3216-1/ to see the changes. Benchmarks is based on merging with master (1dda6e1) |
No description provided.