[Fwd: Re: RFR (M/L): 8010722 assert: failed: heap size is too big for compressed oops]

[Stefan Karlsson]

On 09/05/2013 08:54 AM, Thomas Schatzl wrote:
> Hi,
>
> On Thu, 2013-09-05 at 12:00 +1000, David Holmes wrote:
>> On 5/09/2013 1:19 AM, Thomas Schatzl wrote:
>>> On Wed, 2013-09-04 at 18:22 +1000, David Holmes wrote:
>>>> On 4/09/2013 4:27 PM, Thomas Schatzl wrote:
>>>>> On Wed, 2013-09-04 at 10:59 +1000, David Holmes wrote:
>>>>>> On 3/09/2013 7:59 PM, Thomas Schatzl wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>>       I updated the change for 8010722 per the suggestions of David and
>>>>>>> Stefan and Bengt.
> [...]
>>>>>> That in itself was okay but you still call this from inside the
>>>>>> arguments processing code - which is what I object to.
>>>>> Sorry, I misunderstood then. You suggest something like:
>>>>>
>>>>>      os::init()
>>>>>      Arguments::parse()
>>>>>      os::init_ergo();
>>>>>      Arguments::do_ergonomics()
>>>>>      os::init_2();
>>>>>
>>>>> in Threads::create_vm() where the os:init() methods are called?
>>>> Yes - if that is feasible.
>>> Sure. I like the idea.
>>>
>>> New webrev at http://cr.openjdk.java.net/~tschatzl/8010722/webrev.5/
>> So you have just split parse() in two at the point where you need to
>> inject the os::init_large_pages. That's okay but I don't think the name
>> apply_ergonomics accurately reflects everything that happens in that
>> second function. Further, given that os::init2 is specifically defined
>> as being invoked after arguments have been parsed, it would now be
> I changed the comment for init_2 to "Called after command line parsing
> and VM ergonomics processing" already.
>
>> unclear as to whether it could come before Arguments::apply_ergonomics
>> (it can't!). In that light I'd prefer see very generic naming eg parse()
>> and parse2(), or parse_phase1(), parse_phase2().
> Imo almost all code in the apply_ergonomics() method is about
> configuring/setting VM flags depending on the environment (i.e. what I
> loosely consider ergonomics), so I still think it is appropriate.
>
> If you really think otherwise I will change that, I have no overly
> strong opinion here - but the code does not do any command line parsing
> in the apply_ergonomics()/parse_phase2() method so it seems an even
> bigger misnomer.

I'm not sure I like the way the naming turned out. Arguments::parse() 
still does "ergonomics" and os::init_ergo() is just misleading. However, 
if you can't figure out better names I'll not block this.

Here are some style comments:

http://cr.openjdk.java.net/~tschatzl/8010722/webrev.5/src/share/vm/gc_implementation/g1/heapRegion.hpp.frames.html

Could you add an empty line between 358 and 359.
  358   static size_t max_region_size();
  359   // It sets up the heap region size (GrainBytes / GrainWords), as
  360   // well as other related fields that are based on the heap region
  361   // size (LogOfHRGrainBytes / LogOfHRGrainWords /
  362   // CardsPerRegion). All those fields are considered constant
  363   // throughout the JVM's execution, therefore they should only be set
  364   // up once during initialization time.
  365   static void setup_heap_region_size(size_t initial_heap_size, 
size_t max_heap_size);


http://cr.openjdk.java.net/~tschatzl/8010722/webrev.5/src/share/vm/gc_implementation/parallelScavenge/parallelScavengeHeap.hpp.frames.html

Can you start your comments with capitals? You need to go through the 
entire patch and look at your comments. One example:
   89   // return the (conservative) maximum heap alignment
   90   static size_t conservative_max_heap_alignment() {
   91     return intra_heap_alignment();
   92   }

Why remove the const qualifier? Can you cast a way the constness in the 
caller instead?
  130   static size_t intra_heap_alignment() { return 64 * K * 
HeapWordSize; }


http://cr.openjdk.java.net/~tschatzl/8010722/webrev.5/src/share/vm/memory/collectorPolicy.hpp.udiff.html

Just a comment. After your changes 
GenCollectorPolicy::compute_max_alignment() is now a static function 
that doesn't do any kind of policy work anymore.


http://cr.openjdk.java.net/~tschatzl/8010722/webrev.5/src/share/vm/memory/genCollectedHeap.hpp.udiff.html

I think this should be named conservative_max_heap_alignment().
+  // return the (conservative) maximum heap alignment
+  static size_t max_heap_alignment() {
+    return Generation::GenGrain;
+  }


http://cr.openjdk.java.net/~tschatzl/8010722/webrev.5/src/share/vm/runtime/arguments.cpp.udiff.html

I think you should leave the INCLUDE_ALL_GCS block at the end of the 
include lists, just to be consistent with includes in most of our other 
files.
+#if INCLUDE_ALL_GCS
+#include 
"gc_implementation/concurrentMarkSweep/compactibleFreeListSpace.hpp"
+#include "gc_implementation/g1/g1CollectedHeap.inline.hpp"
+#include "gc_implementation/parallelScavenge/parallelScavengeHeap.hpp"
+#endif // INCLUDE_ALL_GCS


http://cr.openjdk.java.net/~tschatzl/8010722/webrev.5/src/share/vm/runtime/arguments.hpp.frames.html

Excessive amount of spaces in line 437.
  437   static size_t conservative_max_heap_alignment()       { return 
_conservative_max_heap_alignment; }
  438   // return the maximum size a heap with compressed oops can take
  439   static size_t max_heap_for_compressed_oops();


http://cr.openjdk.java.net/~tschatzl/8010722/webrev.5/test/gc/arguments/TestUseCompressedOopsErgoTools.java.html

I think it would be easier to understand the matcher logic if it was 
written as: return m.group(1).equals("true")

  165     String match = m.group();
  166     return match.substring(match.lastIndexOf(" ") + 1, match.length()).equals("true");


Otherwise this looks good as far as I can see. It's a step in the right 
direction to resolve our circular dependencies in the initialization code.

thanks,
StefanK

>
> Thomas
>