Fix #1424 - redundant null check in JDK maps - #1425

Thank you for the PR! but we need to investigate why those checks are there in the first place, there's a chance it was some compatibility fix, or it can be, indeed, just a strange mistake

@elucash Did you already have time to look into this? We use Immutables in various projects and we cannot update Immutables in them if they use maps because Spotbugs complains about it. And our build pipeline is configured to fail if Spotbugs finds any errors.

I'll get back to it asap (1-2 days), sorry for delay

No worries, thank you

Ok, I guess I figured it out. So the older merged PR tried to improve message when a value is null by including a key there. The trick is that we would only incur string concatenation cost (if that is something important to someone) if value is null. In the case it is null, we kinda know that requireNonNull will always fail with message we concatenate (obviously if we just pass a constant string with no +, we don't care). Your PR removes null check, but now we concatenate message each time regardless if we will fail on null or not. We would just write if (value == null) throw new NullPointerException("value for key:" + k), right ? But there are layers. You can customize requireNonNull to throw own exceptions (like new BillionDollarsWithdrawnsFromYourAccountMistakeException("Tony Hoare's fault"), (if someone reading this is interested, see value-fixture/src/org/immutables/fixture/generatorext ). Obscure, yes, but don't want to break that either.
First thing that comes to mind is smth like [requireNonNull type](value, value == null ? "[v.name] value for key: " + key : "value") (the last string literal is, obviously, can be arbitrary, as not used). Is jmpne instruction better than string concat? who knows, but it seems like can preserve the idea. You can try this option or propose some other fix along the lines, i.e. I'll merge changed PR

I updated the PR with the fix proposed by you. I now get the idea behind the initial change. However, if you have concerns about the performance impact, my solution still is worse than before. The string concationation is now only done if the value is null but the requireNonNull method will still be executed. The approach before my change did not execute it in case value == null.

But honestly, I think the performance impact of the string concatination and the extra method call is neglectable. The Java Language Specification says the following about string concationation:
"To increase the performance of repeated string concatenation, a Java compiler may use the StringBuffer class or a similar technique to reduce the number of intermediate String objects that are created by evaluation of an expression"
So depending on the implementation the compiler may keep the concationation as cheap as possible.

In the end I think there is a trade-off between performance and clean code. In my opinion, the negative impact of my initial proposal is neglectable and the generated code does indeed look strange without having the background knowledge you provided here. I would prefer my inital proposal because it looks cleaner but the final decision is up to you. I just wanted to share my opinion on that topic.

If you think that my second proposal meets your requirements, I'm fine with that, too. Spotbugs does not complain about that construct.

Thank you for updating the PR! As performance goes, I'll tell that, of course, we all can be wrong, speculate too much without actually checking with sophisticated tools (like JMH etc), but, probably, "average performance nitpicker" will expect some tricks from that code. In particular requireNonNull call is expected to be inlined, so you'll have an an equivalent to if (== null) format message and throw exception, so you'll have a jmpne (or some other jump) in the for loop. Then, we add another jump with that ternary operator in the PR. The expectation is that 2 relatively short jumps in the loop, which expected to rarely happen, can further help the code where optimized machine instructions can be emitted by JIT and rely on rare branch mispredictions etc. And it's also "expected" that all of this would be more optimal than letting strings concatenate on every iteration. Just for anyone reading, here's my understanding of that StringBuffer optimization: that note that you're referring to is for the typical case where

String x = "a" + b + "c" + d;
// gets "desugared" into
String x = new StringBuilder().append("a").append(b).append("c").append(d).toString();
// (i.e. Java 8 compiler would emit bytecode equivalent to the java line above)

// But I've never seen (may be wrong, but..) about anything like this
String x = "";
for (int i : codesArray) x += ":" + i;
// being turned into below, so people would manually turn it into
StringBuilder xb = new StringBuilder();
for (int i : codesArray) xb.append(":").append(i);
String x = xb.toString();

And the last example is irrelevant in our case, as we need concatenated value on each iteration, so if StringBuilder would be indeed used, we would have new StringBuilder object + its inner char array buffer + resulting string allocated on each iteration (even if we expect no buffer resize and reallocation). So the tradeoff is that 2 jump instructions is definitely better than 3 (or more) object allocations (for each iteration). Furthermore, newer JVM (was it since 9 or 11? don't remember) tend to compile string concatenation to indy, i.e. invokedynamic instruction with the whole boostrap-method/meta-factory machinery. What they do is essentially no longer use StringBuilder directly, but delay that converting till it will be first encountered in runtime so it can be optimized in a "best" way possible now or in the future. While this sounds cool, we don't want to incur all this cost either, especially the first time you hit that call-site it will be non-insignificant performance penalty.
Our builder code is expected to be executed during deserialization and other bulk data reading operations where some mindfulness about performance is something our users expect. We cannot guarantee anything without rigorous performance testing, but at least on the surface, on review level, our generated code is expected to be good enough for common operations.