RFR: String Density/Compact String JEP 254

[Tobias Hartmann]

Hi Roland,

thanks for the review!

On 07.10.2015 15:28, Roland Westrelin wrote:
>> http://cr.openjdk.java.net/~thartmann/compact_strings/webrev/hotspot
> 
> I looked at the compiler related files only.
> 
> intrinsicnode.hpp
> 
> A comment would be nice:
> 
> 49   typedef enum ArgEncoding { LL, LU, UL, UU, none } ArgEnc;

Yes, that enum is a little bit cryptic. I added a comment describing it's purpose.
 
> The (ASSERT only) comment should be removed:
> 
> 53   virtual uint size_of() const; // Size is bigger (ASSERT only)

Done.
 
> library_call.cpp
> 
> In LibraryCallKit::inline_hasNegatives(), don’t you need to test for too many Deoptimization::Reason_intrinsic before intrinsifying?

Right, I added a check to bail out if we hit too many traps.

> Shouldn’t LibraryCallKit::inline_string_toBytesU() and LibraryCallKit::inline_string_getCharsU() use an ArrayCopyNode (maybe an improvement/cleanup for later)?
> 
> Same is true for PhaseStringOpts::arraycopy

The problem with using an ArrayCopyNode is that it requires 

  // (2) src and dest arrays must have elements of the same BasicType

otherwise PhaseMacroExpand::generate_slow_arraycopy() is used. Since for ..toBytesU() and ..getCharsU() we are copying from char[] to byte[] or vice-versa, we cannot use ArrayCopyNode without modifying the macro expansion.

I think the same applies to PhaseStringOpts::arraycopy() where we are copying between two byte arrays because the ArrayCopyNode does not "know" that we can always copy chars since the offsets are guaranteed to be char aligned.

I created a JEP subtask to keep track of this:

https://bugs.openjdk.java.net/browse/JDK-8139132

We may convert this to an "enhancement" after integrating the changes.

> macroAssembler_sparc.cpp 
> 
> I don’t think you should reference c2 stuff outside c2:
> 
> 4440   if (ae == StrIntrinsicNode::LU || ae == StrIntrinsicNode::UL) {

I added '#ifdef COMPILER2' statements to guard the C2 specific string methods in macroAssembler_sparc.cpp.

Here is the incremental webrev:
http://cr.openjdk.java.net/~thartmann/compact_strings/incremental/webrev.03/

I updated the overall webrev in-place:
http://cr.openjdk.java.net/~thartmann/compact_strings/webrev/hotspot/

Thanks,
Tobias