RFR (L): 8060025: Object copy time regressions after JDK-8031323 and JDK-8057536

[Thomas Schatzl]

Hi Mikael,

  thanks for the review, comments inline.

On Wed, 2014-12-03 at 16:15 +0100, Mikael Gerdin wrote:
> Hi Thomas,
> 
> On 2014-12-03 13:41, Thomas Schatzl wrote:
> > Hi all,
[...]
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8060022
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8060025/webrev.0/
> 
> Overall I think this is a good change. I'm not particularly fond of the 
> names InCSetState and in_cset_state_t but I don't really have a good 
> suggestion to replace it. Perhaps the integral type typedef could be 
> moved inside InCSetState and be named something like InCSetState::value.

I will give changing this a try, I was not sure either what is better,
and when I started once doing that it just seemed too much effort for no
gain.

> in g1CollectedHeap.hpp
> Please, let's try to avoid putting more implementation code than 
> necessary in g1CollectedHeap.hpp, please put par_allocate_during_gc, 
> alloc_buffer_stats, desired_plab_sz in g1CollectedHeap.inline.hpp
> 
> can desired_plab_sz do
> alloc_buffer_stats(dest).desired_plab_sz() instead of duplicating
> the switch statement?
> 
> in g1CollectedHeap.cpp
> check_cset_fast_test should be PRODUCT_RETURN_(true) in the hpp.

Will fix these.

> in g1EvacFailure.hpp
> you can compare the result of m->decode_pointer() with obj_addr to avoid 
> the cast to oop. (does this micro-optimization truly influence the evac 
> failure processing performance?)

If you compare to actual evac time: no, evac failure so slow that this
won't affect performance. Mostly did this change to be the same as the
code in G1ParCopyClosure::do_oop_work().

I will remove it as it's not really related to the change anyway.

> in g1Allocator.hpp
> class InCSetState does not really seem to belong in g1Allocator.hpp, 
> have you considered adding it to a new, separate header file?
> the class G1InCSetStateFastTestBiasedMappedArray could also be moved to 
> this new file.

Will do.

Thanks,
  Thomas