RFR: 8141501: Problems with BitMap buffer management

[Stefan Karlsson]

Hi Kim,

Updated webrevs:
http://cr.openjdk.java.net/~stefank/8141501/webrev.03.delta
http://cr.openjdk.java.net/~stefank/8141501/webrev.03

Comments inlined:

On 2016-04-30 19:01, Kim Barrett wrote:
>> On Apr 28, 2016, at 9:23 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>>
>> Hi all,
>>
>> I decided to restructure the allocation code a bit to get better separation between the bitmap allocation logic and the actual bitmap memory allocators.
>>
>> http://cr.openjdk.java.net/~stefank/8141501/webrev.02.delta
>> http://cr.openjdk.java.net/~stefank/8141501/webrev.02
> Generally looks good.

Thanks.

>
> Just a few mostly minor comments or suggestions.  The only really
> important one is about ~G1RegionToSpaceMapper.
>
> ------------------------------------------------------------------------------
> src/share/vm/c1/c1_GraphBuilder.cpp
>   355 void BlockListBuilder::mark_loops() {
>   356   ResourceMark rm;
>   357
>   358   _active.initialize(BlockBegin::number_of_blocks());
>   359   _visited.initialize(BlockBegin::number_of_blocks());
>
> Consider (possibly debug-only) resizing _active and _visited to zero
> at the end of the function, to avoid leaving dangling pointers when
> the ResourceMark is exited.

Done.

>
> ------------------------------------------------------------------------------
> src/share/vm/c1/c1_Instruction.hpp
> 1721   void set_live_in       (ResourceBitMap map)    { _live_in = map;        }
> 1722   void set_live_out      (ResourceBitMap map)    { _live_out = map;       }
> 1723   void set_live_gen      (ResourceBitMap map)    { _live_gen = map;       }
> 1724   void set_live_kill     (ResourceBitMap map)    { _live_kill = map;      }
> 1725   void set_fpu_register_usage(ResourceBitMap map) { _fpu_register_usage = map; }
>
> By-value parameters seem like unnecessary (shallow) copies.  Could
> these be passed by const-ref?

Done.

>
> ------------------------------------------------------------------------------
> src/share/vm/c1/c1_LinearScan.cpp
>   718     block->set_live_in  (ResourceBitMap(live_size)); block->live_in().clear();
>   719     block->set_live_out (ResourceBitMap(live_size)); block->live_out().clear();
>
> Consider instead
>
>    block->live_in().reinitialize(live_size);
>    block->live_out().reinitialize(live_size);

I intend to remove the call to clear() with the fix for this bug:
https://bugs.openjdk.java.net/browse/JDK-8155638

So, I'd like to do this suggestion as a part of that change.

>
> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/g1CardLiveData.cpp
>   400   G1ClearCardLiveDataTask(BitMapView bitmap, size_t num_tasks) :
>
> Pass bitmap by const-ref?

Done.

>
> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/g1RegionToSpaceMapper.hpp
> [Removed]
>    65   virtual ~G1RegionToSpaceMapper() {
>    66     _commit_map.resize(0, /* in_resource_area */ false);
>    67   }
>
> This changes this base class's destructor from public virtual to
> public non-virtual, introducing risk of unintended slicing.

Fixed.

>
> ------------------------------------------------------------------------------
> src/share/vm/utilities/bitMap.hpp
>   182   idx_t size_in_bytes() const { return size_in_words() * BytesPerWord; }
>
> Perhaps instead use
>
>    return calc_size_in_bytes(size());

Done.

>
> ------------------------------------------------------------------------------
> src/share/vm/utilities/bitMap.hpp
>   163   // Protected constructor.
>
> Mention destructor too?

Done.

Thanks,
StefanK

>
> ------------------------------------------------------------------------------
>