[9] RFR(S): 8144212: JDK 9 b93 breaks Apache Lucene due to compact strings

[Vladimir Kozlov]

On 1/7/16 6:52 AM, Tobias Hartmann wrote:
> Hi Vladimir,
>
> On 07.01.2016 00:58, Vladimir Kozlov wrote:
>> Andrew is right.
>
> Yes, he's right that the membar is not needed in this case. I noticed that GraphKit::inflate_string() sets the output memory to TypeAryPtr::BYTES although inflate writes to a char[] array in this case. This caused the subsequent char load to be on a different slice allowing C2 to move the load to before the intrinsic.

Right. It was the root of this bug, see below.

>
> I fixed this for the inflate and compress intrinsics.
>
>> GraphKit::inflate_string() should have SCMemProjNode as compress_string() does to prevent loads move up.
>> StrInflatedCopyNode is not memory node.
>
> Okay, why are above changes not sufficient to prevent the load from moving up? Also, the comment for SCMemProjNode says:

I did not get the question. Is it before your webrev.01 change? Or even with the change?

>
>   // This class defines a projection of the memory  state of a store conditional node.
>   // These nodes return a value, but also update memory.
>
> But inflate does not return any value.

Hmm, according to bottom type inflate produce memory:

StrInflatedCopyNode::bottom_type() const { return Type::MEMORY; }

So it really does not need SCMemProjNode. Sorry about that.
So load was LoadUS which is char load and originally memory slice of inflate was incorrect BYTES.
Instead of SCMemProjNode we should have to change the idx of your dst_type:

set_memory(str, dst_type);

And you should rollback part of changes in escape.cpp and macro.cpp.

>
> Here is the new webrev, including the SCMemProjNode and adapting escape analysis and macro expansion accordingly:
> http://cr.openjdk.java.net/~thartmann/8144212/webrev.01/

In general when src & dst arrays have different type we may need to use TypeOopPtr::BOTTOM to prevent related store & 
loads bypass these copy nodes.

>
> Related question:
> In library_call.cpp, I now use TypeAryPtr::get_array_body_type(dst_elem) to get the correct TypeAryPtr for the destination (we support both BYTES and CHARS). For a char[] destination, it returns:
>   char[int:>=0]:exact+any *
>
> which is equal to the type of the char load.

Please, explain this. I thought string's array will always be byte[] when compressed strings are enabled. Is it used for 
getChars() which returns char array?

Should we also be more careful in inflate_string_slow()? Is it used?

>
> I also tried to derive the type from the array by using dst_type->isa_aryptr(). However, this returns a more specific type:
>   char[int:1]:NotNull:exact *
>
> Using this results in C2 assuming that the subsequent char load is independent and again moving it to before the intrinsic. I don't understand why that is. Shouldn't the second type be a "subtype" of the first type?

It is indeed strange. What memory type of LoadUS? It could be bug.

Thanks,
Vladimir

>
> Thanks,
> Tobias
>
>
>> Thanks,
>> Vladimir
>>
>> On 1/6/16 5:34 AM, Andrew Haley wrote:
>>> On 01/06/2016 01:06 PM, Tobias Hartmann wrote:
>>>
>>>> The problem here is that C2 reorders memory instructions and moves
>>>> an array load before an array store. The MemBarCPUOrder is now used
>>>> (compiler internally) to prevent this. We do the same for normal
>>>> array copys in PhaseMacroExpand::expand_arraycopy_node(). No actual
>>>> code is emitted. See also the comment in memnode.hpp:
>>>>
>>>>    // Ordering within the same CPU.  Used to order unsafe memory references
>>>>    // inside the compiler when we lack alias info.  Not needed "outside" the
>>>>    // compiler because the CPU does all the ordering for us.
>>>>
>>>> "CPU does all the ordering for us" means that even with a relaxed
>>>> memory ordering, loads are never moved before dependent stores.
>>>>
>>>> Or did I misunderstand your question?
>>>
>>> No, I don't think so.  I was just checking: I am very aware that
>>> HotSpot has presented those of use with relaxed memory order machines
>>> with some interesting gotchas over the years, that's all.  I'm a bit
>>> surprised that C2 needs this barrier, given that there is a
>>> read-after-write dependency, but never mind.
>>>
>>> Thanks,
>>>
>>> Andrew.
>>>