Request for Review (s) - 8133023: ParallelGCThreads is not calculated correctly

[Kim Barrett]

On Dec 1, 2015, at 1:14 AM, David Holmes <david.holmes at oracle.com> wrote:
> 
> On 1/12/2015 1:04 PM, Kim Barrett wrote:
>> On Nov 25, 2015, at 9:11 PM, David Holmes <david.holmes at oracle.com> wrote:
>>> 
>>> We already have VM_Version::early_initialize() that can/should be used for this if possible.
>> 
>> VM_Version::early_initialize doesn’t work for this, because
>> determine_features (sparc) examines some command-line arguments, and
>> those aren't parsed until a later stage of Threads::create_vm.
>> 
>> Specifically, determine_features (sparc) is presently looking at two
>> CLAs: UseV8InstrsOnly (develop) and UseNiagaraInstrs (product).  I
>> suspect UseV8InstrsOnly has served its purpose and could be purged.
>> But calling determine_features before argument parsing would
>> unintentionally ignore UseNiagaraInstrs.
>> 
>> Calling determine_features from os::init_before_ergo doesn't have this problem.
>> 
>> [I missed this in my review of Jon’s original change to use early_initialize.]
> 
> Thanks Kim. Still makes me wonder if it isn't something else that is in the wrong place - the initialization dance is very tricky and I hate to see yet-another-vm-version-init function added. Makes me wonder whether determine_features should only look at what the hardware provides, and then after argument processing is complete (or appropriate portion thereof) we update the features that should actually be used? - but that ship sailed long ago.

I agree this seems kind of suspect.  Large program genesis can be so
much fun.

I think the suggestion of calling determine_features early and then
clobbering the result later should work.  I don't much like the
modification of the features set far away from it's initialization
though.  And that would still require the introduction of some
platform-specific code into about the same place Jon is proposing, in
order to do the feature bashing.

Conflating the execution platform with the target platform is a very
appealing simplification, which unfortunately starts unraveling with
the introduction of things like UseNiagaraInstrs (or AOT or cross
compilation, on a much wider scale).

A better solution might be to move determine_features early (in
early_initialize) and change places that should be dependent on
UseNiagaraInstrs but are presently using _features to (more)
explicitly depend on UseNiagaraInstrs, rather than expecting _features
to be affected by UseNiagaraInstrs.  But that's probably at least
somewhat hard.

Another approach would be move determine_features early (in
early_initialize) and to ignore UseNiagaraInstrs and UseV8InstrsOnly.
I don't know whether either serves any really useful purpose anymore.
UseNiagaraInstrs is a product option though.

Other than the last of those (ignore presumably obsolete options), I
don't think I like any of the choices better than what Jon is
proposing.