SI-6370 changed ListMap apply method to produce correct error message #2009
Conversation
… when a key is not found Current implementation of apply uses tail recursive apply0 to get a value from a key. apply0 relies on tail method to iterate over all keys.When the list gets to its end, tail produces an 'empty map' message in its exception, which is thrown by ListMap. This change catches this exception in the apply method and change the exception message to a more appropriate 'key not found' message Signed-off-by: Vinicius Miana <vinicius@miana.com.br>
|
One solution would be to override A better solution would be to modify the body of Might be good to write a couple of microbenchmarks to verify how this affects traversal performance. |
|
That was my first solution, but I did not want to add this extra if in the recursive function. |
|
You can. |
|
The problem with private tail0 is that it would not have cur.key available for a nice error message, unless we pass that just for that reason. I would lean towards the apply0 if you feel that the try {} catch is not acceptable and the loss is performance is negligible. It is the solution with the cleaner code. |
|
If you intend to add |
|
Actually did not went that far, as I did not like the idea to pass the current key for a nice message.
|
|
Ok, in that case, we should justify the costs of the extra |
|
There is no statistically meaningful difference between the try-catch and the if in terms of time to do up to 100.000 calls to apply. |
|
And here the two versions of the apply without and with the try catch wrapper: |
|
Conclusion: the difference is performance is not significant, the extra if adds just one instruction in the recursive call and in the end it does not make any difference. I will amend the commit and change it to the recursive if, ok? |
|
I really appreciate you taking the time to inspect the bytecode. However, since there are only a few people on the globe who know so much about the JIT and the JVM to accurately predict the performance effect of adding instructions in particular locations in the bytecode, the conclusion that adding a single instruction makes no difference is at best shady. I therefore warmly recommend a microbenchmark to verify these assumptions, in particular 2 of them:
There are some docs on writing precise microbenchmark, see here for example: http://docs.scala-lang.org/overviews/parallel-collections/performance.html Or did I misunderstand and you already ran a benchmark? Could you link to a gist? Thanks |
|
I would say that inspect that byte-code is not only shady, but naive. I put up the bytecode, thinking that you could be one of those people or someone else on this list. I already did a benchmark: |
|
You honour me greatly, but I'm not one of those :) Ok, looking forward to see the results. Thanks a lot! |
|
I changed the tests following the instructions you posted, but I did not get anything statiscally meaningful. You can see 2 consecutive runs of the original ListMap for big sizes: |
|
I just had a look at your benchmarking results. It appears to me that for small sizes, the apply0/if option seems a bit faster. This is important, since some high-throughput applications create many, many small collections (the Scala compiler is one example; I'm not saying Thanks. |
|
Yep, the extra Thanks a lot! |
|
I would also prefer the solution without the try/catch. Please open a new PR once the 2.10.x branch becomes the target for fixes going into 2.10.2 (I'll probably branch for 2.10.1 on Friday Feb 8). I think we're too close to RC1 to change this now anyway -- sorry. |
Current implementation of apply uses tail recursive apply0 to get a value from a key. apply0 relies on tail method to iterate over all keys.When the list gets to its end, tail produces an 'empty map' message in its exception, which is thrown by ListMap. This change catches this exception in the apply method and change the exception message to a more appropriate 'key not found' message
review by @phaller