RFR (L): 7088419 : Use x86 Hardware CRC32 Instruction with java.util.zip.CRC32 and java.util.zip.Adler32

[Christian Thalinger]

On Jun 26, 2013, at 12:31 PM, David Chase <david.r.chase at oracle.com> wrote:

> 
> On 2013-06-26, at 2:17 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
> 
>> For Doxygen parameters you should use @param:
>> 
>> http://www.stack.nl/~dimitri/doxygen/manual/commands.html#cmdparam
>> 
>> If we have a Doxygen method comment we don't need comments in header files:
> 
> I will look into Doxygen, but do we really "not need" those comments?

Having two comments is subject to bit-rot.  Just look at our sources.

> Some of us don't use Doxygen.

I don't use it either but it's a defacto-standard for comments and (some) IDEs pick it up.  It's definitely better than anything else.

> 
>> src/share/vm/prims/jvm.cpp:
>> 
>> +   PUTPROP(props, "sun.zip.clmulSupported", X86_ONLY(!UseCRC32Intrinsics && UseAVX && UseCLMUL ? "true" :) "false");
>> 
>> When you say "No changes to JDK: this is all in the compiler." does it mean this review has no JDK changes or are there no JDK changes at all?
> 
> There are no JDK changes at all.
> Probably the parameter-putting should go, since otherwise that means a new unwanted property.
> And even if it doesn't, it should be called sun.zip.intrinsicCRC32 or some such.
> And there's a bug in the parameter setting anyhow that I thought I had fixed -- should not negate UseCRC32Intrinsics.
> (Somehow, I captured an old version of this file in my webrev; it's not relevant to running tests.)

Yes, remove it.

> 
>> src/cpu/x86/vm/templateInterpreter_x86_32.cpp:
>> 
>> I thought I've changed this already but apparently I didn't:
>> 
>>     case Interpreter::java_lang_math_exp     : entry_point = ((InterpreterGenerator*)this)->generate_math_entry(kind);     break;
>> 
>> Can we please add a local variable for (InterpreterGenerator*)this (on all architectures)?
> 
> Would "ig_this" be happy-making?

Oh yeah!  Even ig would do it.

-- Chris

> 
> David
>