Memory Segment efficient array handling

Thu Jun 17 17:59:20 UTC 2021

On 17/06/2021 18:56, leerho wrote:
> Maurizio,
> Wow! Thanks!
>
> You clearly put a lot of work into these changes!  You completely 
> rewrote the test class using tools that I was not aware of as well as 
> simplifications of the ultimate code.  I was pleased that at least the 
> basic concept of the tests survived.

Hopefully the test logic should be the same. The only think in the test 
which did not survive was the fact that for float and doubles, the old 
logic started the segment with Math.PI. The new logic just works in 
bytes and always starts at 0.

We could tweak that if required, but IMHO initializing segment with 
bytes should always be good enough?

>
> I am very grateful for the time you spent on this!

You're welcome - thanks a lot for the help on this one!

Maurizio

>
> Cheers,
> Lee.
>
>
>
> On Thu, Jun 17, 2021 at 8:44 AM Maurizio Cimadamore 
> <maurizio.cimadamore at oracle.com 
> <mailto:maurizio.cimadamore at oracle.com>> wrote:
>
>     I've finalized the PR, it is now a full PR which is ready for
>     review. I
>     did some passes over the code and fix some things here and there,
>     hopefully nothing too controversial.
>
>     Cheers
>     Maurizio
>
>     On 16/06/2021 16:56, Maurizio Cimadamore wrote:
>     > Hi Lee,
>     > I've updated the PR with your latest changes. Everything works
>     fine -
>     > there were some trailing whitespaces that I fixed, other than
>     that the
>     > test works ok.
>     >
>     > Maurizio
>     >
>     > On 15/06/2021 20:06, leerho wrote:
>     >> Maurizio,
>     >> I have updated the *MemoryCopy* and *TestMemoryCopy* to include
>     >> references to the new *MemorySegment::CopyFrom(...)* and with byte
>     >> swap capabilities.
>     >>
>     >> I have been able to test the code in my local environment using a
>     >> mock substitute for the new CopyFrom(), but alas, I could not get
>     >> Jtreg to work, so i am sending you the code as updated in
>     >> https://github.com/leerho/mcimadamore_panama-foreign
>     <https://urldefense.com/v3/__https://github.com/leerho/mcimadamore_panama-foreign__;!!GqivPVa7Brio!JGhJxrE53VOn-cTxQVrSqgTu2RNga-LHbmFznJnCRboLnzElMBFILU_QjDRyz-R-HaFEm0A$>
>
>     >>
>     <https://urldefense.com/v3/__https://github.com/leerho/mcimadamore_panama-foreign__;!!GqivPVa7Brio!MZMNJXVSSBqQxv9G2nXnY-wpuBdgPpwIvBCl6CuzkZLWKaRjEAPnl1FHib5FeUyX3rF3rRU$
>     <https://urldefense.com/v3/__https://github.com/leerho/mcimadamore_panama-foreign__;!!GqivPVa7Brio!MZMNJXVSSBqQxv9G2nXnY-wpuBdgPpwIvBCl6CuzkZLWKaRjEAPnl1FHib5FeUyX3rF3rRU$>>
>     >>
>     >> Please test it, when you have a chance.
>     >>
>     >> Lee.
>     >>
>     >>
>     >> On Tue, Jun 15, 2021 at 5:43 AM Maurizio Cimadamore
>     >> <maurizio.cimadamore at oracle.com
>     <mailto:maurizio.cimadamore at oracle.com>
>     >> <mailto:maurizio.cimadamore at oracle.com
>     <mailto:maurizio.cimadamore at oracle.com>>> wrote:
>     >>
>     >>
>     >>     >> The reason why copying is slower for small segments is
>     that, I
>     >>     think, my
>     >>     >> benchmark iterations run in 4-5ns and, at these levels,
>     you are
>     >>     >> sensitive to the cost of the liveness checks. There's
>     not a lot
>     >>     we can
>     >>     >> do to eliminate that - it's the price of safety.
>     >>     > OK that's true. But in my own Unsafe.copyMemory code wasn't
>     >>     doing MemorySegment.address().toRawLongValue() not also
>     doing the
>     >>     liveness check?
>     >>     True - so the liveness check probably that won't make a lot of
>     >>     difference in your case. But note that the bound checks are
>     still
>     >>     applied in MemorySegment::copyMemory.
>     >>     >
>     >>     >> The good news is that in the case of heap segment
>     wrappers, we
>     >> know
>     >>     >> exactly that the size is gonna fit inside an int (it's
>     an array
>     >>     after
>     >>     >> all!), so we can remove the check. If we remove the flag
>     check,
>     >>     and just
>     >>     >> set the segment to always be treated as "SMALL", we get the
>     >>     following
>     >>     >> results:
>     >>     > Is this really true? What about a huge long[] array, that
>     could
>     >>     be (2^31-8 maximum index) * 8 bytesPerLong = 16 gigabytes?
>     (or are
>     >>     those limited by the Java standard in maximum array size?)
>     >>     in the PR I've submitted I only tweaked byte segment
>     wrappers. We
>     >>     could
>     >>     resort to more complex internal representation for other heap
>     >>     segments,
>     >>     but I believe the true way forward is to wait for the long
>     >>     optimizations, and to completely remove the SMALL segment
>     hacks.
>     >>     >
>     >>     > Sorry, I missed that class. I was opening the pull
>     request and
>     >>     only looked at the red/green changes in MemorySegment. I missed
>     >>     the first class.
>     >>     >
>     >>     > The methods provided there exactly replace my three shapes in
>     >>     the code. And on top they allow to swap. Side note:
>     MemoryCopy in
>     >>     the current PR has the swapping code commented out.
>     >>     Yeah - Lee is working on that MemoryCopy class (and associated
>     >>     test). I
>     >>     have provided the "swappy" copyFrom overload in
>     MemorySegment, I
>     >>     believe
>     >>     Lee will add support for that soon.
>     >>     >
>     >>     > Last question: why does
>     >> https://bugs.openjdk.java.net/browse/JDK-8223051
>     <https://bugs.openjdk.java.net/browse/JDK-8223051>
>     >>     <https://bugs.openjdk.java.net/browse/JDK-8223051
>     <https://bugs.openjdk.java.net/browse/JDK-8223051>> not help for
>     >>     the integer vs. long loop variables? Does this not allow to
>     remove
>     >>     the small vs. large segment distinction? I thought this
>     change in
>     >>     hotspot was added to support long loops, if it is in fact
>     "small".
>     >>
>     >>     My understanding is that the change has been split in multiple
>     >>     chunks.
>     >>     The chunk we mostly depend on is this:
>     >>
>     >> https://bugs.openjdk.java.net/browse/JDK-8259609
>     <https://bugs.openjdk.java.net/browse/JDK-8259609>
>     >>     <https://bugs.openjdk.java.net/browse/JDK-8259609
>     <https://bugs.openjdk.java.net/browse/JDK-8259609>>
>     >>
>     >>     (in fact I'm coordinating with Roland, the author of that work,
>     >>     and he's
>     >>     testing the patch against a version of panama which doesn't
>     have
>     >>     workarounds for small segments).
>     >>
>     >>     I did run all Panama benchmarks after JDK-8223051 was
>     pushed, and
>     >>     removed various workarounds, but  there was still a
>     performance gap.
>     >>     Roland reassured me that the gap should disappear once
>     >> JDK-8259609 is
>     >>     pushed.
>     >>
>     >>     Maurizio
>     >>
>     >>     >
>     >>     > Uwe
>     >>     >
>     >>
>