optimise Btypes descriptor generation #5831
Conversation
96cc3f5 to
29d8466
Compare
| * generally used by descriptor to help build the descriptor. Allows several descriptors to be build from the same stringbuilder | ||
| * @param sb | ||
| */ | ||
| private[BTypes] def buildDescriptor(sb: java.lang.StringBuilder): Unit |
There was a problem hiding this comment.
I'd prefer to use a pattern match here instead of virtual dispatch. With an overload override def descriptor(sb: java.lang.StringBuilder): String in Class/Array/MethodBType this should be possible.
There was a problem hiding this comment.
Not sure what you are getting at
As I see it you either use a pattern match or virtual dispatch
Getting run of the 'this match { case ...} ' pattern in the code aids encapsulation, safety and performance IMHO
Why an overload? They are behaviourly different?
I am turn as to whether the buildDescriptor should return the descriptor or generate i as a side effect. Either way there are 2 effects, and I could not come up with a a better name before my attention moved
There was a problem hiding this comment.
I think we generally prefer matching on this for sealed data types like BType
There was a problem hiding this comment.
@lrytz I think that it is a very bad idea. I did see large amounts or this elsewhere, and it is on my todo list. Is this personal preference or lightbend policy?
My view -
Given that you dont have warnings as fatal, it is fragile
From a perfomance perspective I would expect it to be non-optimal. Has this been benchmarked? I know that @retronym is a dab hand with JMH :-)
I would suggest that we need some figures to compare and determine what the effective costs and benefits are
Virtual dispatch, particularly from abstract classes rather than traits is very fast. Pattern matching is a sequential operation, slowed with unapply calls. Inner classes (e.g., from the global cake) make this even more onerous
I agree that is some cases it may be appropriate - code share, cold paths, but these paths are hot
There was a problem hiding this comment.
Pattern matching is a sequential operation, slowed with unapply calls
In general, your point is valid, and in some cases the performance does make a difference (e.g. in mapOver #5906 ), but it is important to note that most pattern matching is on case class constructors or plain old instanceOfs and doesn't involve unapply calls. So we should balance readability (which is sometimes enhanced with a pattern match) vs performance by confirming that there is an improvement in benchmarks from a refactoring.
| override def descriptor: String = { | ||
| if (descriptorCache eq null) { | ||
| val sb = new java.lang.StringBuilder(internalName.length + 2) | ||
| buildDescriptor(sb) |
There was a problem hiding this comment.
there will probably be concurrent accesses to this method once we parallelize the backend, no?
There was a problem hiding this comment.
Yes it would be concurrent, but I believe that it is threadsafe
I don't believe that the cost of lock is worth the potential gain in lost CPU
the answer is the same (==) so 2 threads race one will win. Small chance that we will have 2 thread with identical strings, but this is not harmful
There is no chance of a incomplete string being visible to another thread as String constructor has a memory fence
29d8466 to
1bee4de
Compare
1bee4de to
14132b4
Compare
|
I wasn't able to clearly measure an improvement from this change on our benchmark server. That might be because we compile with an oversized heap (initial/max=2G) to try to avoid GC noise in the benchmark run, which could tilt things in favour of the status quo. We're still getting the hang of benchmarking, so this is just a datapoint, not a conclusive answer. |
|
I'm planning on re-profiling this one during the week - hopefully I can add some more clarity on it. |
|
What's the latest on the benchmarks? The window for 2.12.4 is closing soon (sign-off must happen by Wed Sep 27). |
|
I will not have time to met that cut off. Trying to work on the parallelization of the jvm first, but I doubt that will be ready in time either |
|
No problem, thanks for the update. |
|
I'm closing this PR because it has been dormant for a while now 95F8 . Happy to receive a resubmission! |
part of #5815 isolated to aid review
performance result look reasonable. The 2 results are for the 2 commits