Remove redundant calls of toString()
claes.redestad at oracle.com
Wed Apr 30 12:17:46 UTC 2014
On 2014-04-30 13:08, Remi Forax wrote:
> On 04/29/2014 11:20 AM, Claes Redestad wrote:
>> On 2014-04-29 09:31, Remi Forax wrote:
>>> On 04/28/2014 05:43 PM, Claes Redestad wrote:
>>>> On 04/28/2014 08:57 AM, David Holmes wrote:
>>>>> On 28/04/2014 1:05 PM, Otávio Gonçalves de Santana wrote:
>>>>>> In my opinion not, because Objects.requireNonNull is more
>>>>>> readable than
>>>>>> just string.toString. This way is more understandable which field is
>>>>>> required and doesn't impact on performance.
>>>>> An invocation of requireNonNull is potentially more expensive than
>>>>> the implicit null check that happens with foo.toString().
>>>> My thought was that these two would be inlined to the exact same
>>>> thing, so I did a quick test to see what happens when you do
>>>> foo.toString() versus Objects.requireNonNull(foo) on a set of
>>>> randomly generated String's with different amounts of null
>>>> elements(0p: no null entries, 1p: 1% null entries etc):
>>>> Benchmark Mode Samples Mean
>>>> Mean error Units
>>>> s.m.ThrowAwayBenchmark.nullToString0p thrpt 6 356653.044
>>>> 3573.707 ops/ms
>>>> s.m.ThrowAwayBenchmark.nullToString1p thrpt 6
>>>> 353128.903 2764.102 ops/ms
>>>> s.m.ThrowAwayBenchmark.nullToString10p thrpt 6
>>>> 297956.571 9580.251 ops/ms
>>>> s.m.ThrowAwayBenchmark.nullToString50p thrpt 6
>>>> 158172.036 1893.096 ops/ms
>>>> s.m.ThrowAwayBenchmark.nullToString100p thrpt 6
>>>> 18194.614 472.091 ops/ms
>>>> s.m.ThrowAwayBenchmark.requireNonNull0p thrpt 6
>>>> 357855.126 2979.090 ops/ms
>>>> s.m.ThrowAwayBenchmark.requireNonNull1p thrpt 6
>>>> 67601.134 7004.689 ops/ms
>>>> s.m.ThrowAwayBenchmark.requireNonNull10p thrpt 6
>>>> 8150.595 538.970 ops/ms
>>>> s.m.ThrowAwayBenchmark.requireNonNull50p thrpt 6
>>>> 1604.919 220.903 ops/ms
>>>> s.m.ThrowAwayBenchmark.requireNonNull100p thrpt 6
>>>> 820.626 60.752 ops/ms
>>>> Yikes! As long as the value is never null they're inlined nicely
>>>> and neither have the upper hand performance-wise, but as soon as
>>>> you get some null values, Objects.requireNonNull degenerates much
>>>> faster than its foo.toString counterpart. I think this is a JIT
>>>> issue - optimizing exception-paths might not be the highest
>>>> priority, but Objects.requireNonNull is used pretty extensively in
>>>> the JDK and my expectation would be that it shouldn't degrade
>>>> performance when things actually are null now and then.
>>> This is a know issue, I think it's not related to the way the JIT
>>> handle exception path but what is called 'profile pollution'.
>>> Hotspot JITs have two ways to do a null check, either do nothing
>>> (yes nothing) and let the system do a fault and come back from dead
>>> using a signal handler, this solution is named implicit null check
>>> or by doing an explicit null check, i.e a conditional jump.
>>> Implicit null check is faster but if the receiver is null, it cost
>>> you an harm, so the VM profiles receiver to remember if the receiver
>>> of each call can be null or not. The problem is that when you call
>>> foo.toString(), the profile information is associated with the
>>> instruction that does foo.toString() while if the call is
>>> Objects.requireNonNull(foo), the profile associated with the
>>> nullcheck is stored inside the method requireNonNull, so if one call
>>> to requireNonNull in the entire program throw a NPE, the profile
>>> inside requireNonNull now register that it may fail, so for the VM
>>> all calls to requireNonNull may fail. So currently you should not
>>> use requireNonNull is the value is not required to be null*
>> I guess I should have given more details. :-) I ran my micros on a
>> number of forked VMs to ensure I don't get excessive run-to-run
>> variations, which among other things avoids profile pollution. Also I
>> sloppily collected very few samples and only did minimal warmup after
>> I saw that the micros produced scores that were orders of magnitude
>> apart and the values stabilized after less than two seconds: Flags:
>> -f 3 -wi 4 -i 2
>> When I run the micros sequentially in the same VM I see some added
>> degradation for the exceptional cases, but the 0p cases still inline
>> to the optimal(?) 360k ops/ms, so while it seems profile pollution
>> might play in when provoked, HotSpot seems to manage these simple
>> micros well enough.
> Ok, so you have to take a look to the generated assembly code :)
I've discussed the problem with some members of the compiler team who
seem to think they have a pretty good idea about what's happening and
thought I should file an RFE for this:
https://bugs.openjdk.java.net/browse/JDK-8042127 - I suggest we continue
the discussion there and/or start a new thread on this topic.
>>> So the problem is that Hotspot blindly trusts the recorded profile
>>> compared to the profile that can come from the arguments of a call.
>>> The good news is that recently some patches were included in the
>>> jdk9 tree to fix or at least mitigate that issue.
>> I've now run tests on JDK8 FCS and a recent internal build of JDK9
>> and see the same characteristics. How recently are we talking here? :-)
> I don't know, I have seen the patches but not dive into the source
> yet, it's on my todo list, maybe it's not even activated by default.
>>> * read that last sentence again, it seems very logical, no ?
>> While your reading of the method name and suggested approach makes
>> sense right now, the javadoc for Objects.requireNonNull states "this
>> method is designed primarily for doing parameter validation in
>> methods and constructors". To me, "parameter validation" suggests
>> that we're dealing with unknowns and that there's at least some
>> calculated risk that the input might be null, so the intent is really
>> to encourage explicit fail-fast designs, no?
> The great Java Book is clear on that, you shall never try to recover
> from an exception that inherits from RuntimeException :)
> A runtime exception is an exception that developers throw at the head
> of their fellow developers to indicate that they should read the
> fucking manual.
I completely agree with you, however, I still think this is something
that might happen in production systems because of reasons, so while we
can't eliminate the penalty I still think it's a reasonable request to
try and achieve a similarly graceful performance degradation as when
doing implicit null checks.
>> I thus think it makes sense to try and ensure these methods have the
>> same performance characteristics as when provoking an implicit null
>> check by dereferencing an object.
> as I said, I think it's time to take a look to the generated assembly
More information about the core-libs-dev