RFR (M) 8059461: Refactor IndexSet for better performance (preliminary)

[Aleksey Shipilev]

Thanks for taking a look, Vladimir!

This is a change in performance-sensitive part of the compiler, and so I
share your worry. Nothing that's worthwhile is ever easy.

I will attend to your comments later. The methodology for measuring the
compiler footprint seems reasonable.

-Aleksey.

On 11/13/2014 05:09 AM, Vladimir Kozlov wrote:
> I am start worrying about this change.  Nested ResourceMark may cause
> problems.
> 
> PhaseLive::getfreeset() uses thread local resource area for
> _free_IndexSet and PhaseLive::compute() resets it on exit:
> 
>    // Init the sparse arrays for delta-sets.
>    ResourceMark rm;              // Nuke temp storage on exit
> 
> But the rest of IndexSet objects uses indexSet_arena which is
> 'live_arena' in Register_Allocate(). It lives longer then
> PhaseLive::compute() and PhaseLive::compute() may 'nuke' its data
> incorrectly if you use thread local resource area for everything.
> 
> May be you should also separate arenas for BitMap used as IndexSet.
> 
> I think you need to instrument JVM to get correct data. If you use
> separate ResourceArea (as in original code 'live_arena') you can print
> its size at the end of its use scope.
> 
> I would recommend to run with -Xbatch -XX:-TieredCompilation
> -XX:CICompilerCount=1 to get serialized data which can be compared.
> 
> Clean up:
> 
> Remove commented assert in lrg_union() since it is not applicable anymore.
> 
> Fix style in the code you are changing. No spaces in if():
>   if( vec->is_empty() ) return;
> should be
>   if (vec->is_empty()) {
>     return;
>   }
> 
> Thanks,
> Vladimir
> 
> On 11/11/14 11:20 AM, Aleksey Shipilev wrote:
>> Thanks for review, Vladimir!
>>
>> FTR, here's an updated webrev:
>>   http://cr.openjdk.java.net/~shade/8059461/webrev.03/
>>
>> On 11/06/2014 01:58 AM, Vladimir Kozlov wrote:
>>> Aleksey, can you compare average memory consumed by IndexSet before and
>>> after?
>>
>> Any ideas how to estimate this reliably? Current code has some
>> instrumentation, but only about usage, not the concrete footprint. NMT
>> shows very flaky numbers for Nashorn/Octane:
>>
>> $ grep "Compiler" before/*
>>   Compiler (reserved=165KB, committed=165KB)
>>   Compiler (reserved=174KB, committed=174KB)
>>   Compiler (reserved=201KB, committed=201KB)
>>   Compiler (reserved=189KB, committed=189KB)
>>   Compiler (reserved=192KB, committed=192KB)
>>
>> $ grep "Compiler" after/*
>>   Compiler (reserved=162KB, committed=162KB)
>>   Compiler (reserved=167KB, committed=167KB)
>>   Compiler (reserved=198KB, committed=198KB)
>>   Compiler (reserved=240KB, committed=240KB)
>>   Compiler (reserved=188KB, committed=188KB)
>>
>> $ grep "Arena" before/*
>>   Arena Chunk (reserved=10952KB, committed=10952KB)
>>   Arena Chunk (reserved=11433KB, committed=11433KB)
>>   Arena Chunk (reserved=10506KB, committed=10506KB)
>>   Arena Chunk (reserved=10825KB, committed=10825KB)
>>   Arena Chunk (reserved=7179KB, committed=7179KB)
>>
>> $ grep "Arena" after/*
>>   Arena Chunk (reserved=19971KB, committed=19971KB)
>>   Arena Chunk (reserved=20738KB, committed=20738KB)
>>   Arena Chunk (reserved=21090KB, committed=21090KB)
>>   Arena Chunk (reserved=22304KB, committed=22304KB)
>>   Arena Chunk (reserved=14342KB, committed=14342KB)
>>
>> It *looks* like a memory leak in Arenas, but it might be as well the
>> larger peak usage. Or, it might be NMT not telling me the whole story.
>>
>> I am running Footprint tests from our performance regression suites now
>> -- they will show if there is something real.
>>
>>> Why you need initialize_in_resource_arena()? by default BitMap() uses
>>> resource area:
>>>
>>> BitMap(idx_t size_in_bits, bool in_resource_area = true);
>>
>> Yes, pruned that.
>>
>>> Make lrg_union() PhaseConservativeCoalesce class's method.
>>
>> Yes, did that.
>>
>> Thanks,
>> -Aleksey.
>>
>>


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20141117/d6dce4b5/signature.asc>