RFR: 8224202: Speed up Properties.load

[Roger Riggs]

Hi Claes,

Properties.java: 542-547, 636-639 you might find the function Ivan added 
useful:
         newlen =  ArraySupport.newLength(oldlength, min, preferred);

Can it be refactored differently to read the bytes into a char array and
then process that to avoid the processing code duplication.

If not, can you put the two byte and char versions of readLine in 
separate methods,
That massive if..else is ugly. Not sharing most of the code doing the 
same things is quite ugly too.

Roger


On 05/22/2019 01:15 PM, Claes Redestad wrote:
> Reworked after further analysis:
>
> - swapping out char[] for StringBuilder was only a win for the outgoing
>   buffer
> - using locals for oft-accessed variables in readLine is a substantial
>   interpreter-only optimization[1]
> - the skipLF logic can safely be folded into the end-of-line logic,
>   thus removing a test from every non-comment character read
> - splitting apart the InputStream / Reader removes numerous branches
>   and allows some improvements for the common InputStream case - at the
>   price of some code duplication
>
> In total, this patch now means we need to execute almost half as much
> bytecode to load java.security. In my local tests this means we load
> java.security ~1.5-2ms faster.
>
> New webrev: http://cr.openjdk.java.net/~redestad/8224202/open.01/
>
> /Claes
>
> [1] this optimization effort is motivated by startup regressions caused
> by loading java.security earlier during bootstrap in some
> configurations; the number of property files we load is typically small
> and happen early, so optimization focus is thus on making it faster
> during bootstrap, i.e., interpreted performance - if we were optimizing
> for parsing many property files we'd probably need to restructure the
> logic, e.g., splitting out duplicate code into smaller methods etc..
>
>
> On 2019-05-21 12:22, Claes Redestad wrote:
>> Hi,
>>
>> a few smaller optimizations for Properties.load:
>>
>> - when parsing comment lines, we unnecessarily breaks the fast path loop
>> on backslashes, even though a backslash embedded in a comment line 
>> will never have any effect on subsequent logical lines
>>
>> - inside that same loop, we always do two comparisons in the common 
>> case: testing c <= '\r' && c >= '\n' would mean only one comparison 
>> in the common case, since c is very likely to be > '\r' (also when 
>> reading from a byte stream)
>>
>> - we currently store everything into temporary char[]'s which we then
>> instantiate Strings with. Since Compact String this means we often
>> use more temporary storage than necessary, and do unnecessary work
>> packing from char[] to byte[] representation internally; by using
>> StringBuilder encoding transitions is handled more gracefully and is a
>> speed-up in general (both interpreted and compiled) while simplifying
>> the code somewhat.
>>
>> Result is ~5-20% faster depending on how comment-heavy the properties
>> file we're loading is.
>>
>> Bug:    https://bugs.openjdk.java.net/browse/JDK-8224202
>> Webrev: http://cr.openjdk.java.net/~redestad/8224202/open.00/
>>
>> Testing: tier1-3
>>
>> Patch is applied on top of JDK-8224240, patch for which is out for
>> review here: 
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-May/060301.html
>>
>> Thanks!
>>
>> /Claes