Re: RFR: 8343188: Investigate ways to simplify MemorySegment::ofBuffer [v2]

2024-10-31 Thread Maurizio Cimadamore 
On Thu, 31 Oct 2024 10:25:11 GMT, Maurizio Cimadamore  
wrote:

> Do you refer to char buffers backed by Strings?

If so, I wouldn't worry much about that -- after all, the logic in the code 
already detects this condition and throws an exception... so it's still heap 
buffer vs. direct buffer IMHO

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21764#discussion_r1824227728

Re: RFR: 8343188: Investigate ways to simplify MemorySegment::ofBuffer [v2]

2024-10-31 Thread Maurizio Cimadamore 
On Thu, 31 Oct 2024 07:10:29 GMT, Per Minborg  wrote:

>> src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
>>  line 541:
>> 
>>> 539: 
>>> 540: @ForceInline
>>> 541: private static AbstractMemorySegmentImpl arrayFreeSegment(Buffer 
>>> b, long offset, long length) {
>> 
>> the names `arrayFreeSegment` and `arrayBackedSegments` seem a bit confusing. 
>> I'd suggest `ofDirectBuffer` and `ofHeapBuffer`
>
> I called them that initially but there is a subtle difference; there are heap 
> buffers that are not backed by an array. The name arrayFreeSegment could 
> better be arrayLessSegment.

Not sure I follow. The code asks if there's a backing array (accessing base). 
Then:
* if there is a base, it calls `arrayBackedSegment`
* otherwise it calls `arrayLessSegment`.

Now, `arrayBackedSegment` is only implemented for heap buffers. Direct buffers 
throws UOE.
And, inside `arrayLessSegment` we throw if we see a non-direct (=heap) buffer.

So... it seems to me that arrayBacked = heap and arrayLess = direct?

Do you refer to char buffers backed by Strings?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21764#discussion_r1824221715

Re: RFR: 8343188: Investigate ways to simplify MemorySegment::ofBuffer [v2]

2024-10-31 Thread Per Minborg 
On Wed, 30 Oct 2024 18:34:49 GMT, Maurizio Cimadamore  
wrote:

>> Per Minborg has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Update from comments and add benchmark
>
> src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
>  line 546:
> 
>> 544: }
>> 545: final UnmapperProxy unmapper = NIO_ACCESS.unmapper(b);
>> 546: return unmapper == null
> 
> I would feel more at ease if the creation of segments went through 
> SegmentFactories (this is not a new issue in this PR)

https://bugs.openjdk.org/browse/JDK-8343347

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21764#discussion_r1824104624

Re: RFR: 8343188: Investigate ways to simplify MemorySegment::ofBuffer [v2]

2024-10-31 Thread Per Minborg 
> This PR proposes to improve `MemorySegment::ofBuffer` making it more amenable 
> to inlining and generally improving performance.
> 
> Testing successfully on tier1-3

Per Minborg has updated the pull request incrementally with one additional 
commit since the last revision:

  Update from comments and add benchmark

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/21764/files
  - new: https://git.openjdk.org/jdk/pull/21764/files/c430801d..fd9a7b60

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=21764&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21764&range=00-01

  Stats: 104 lines in 6 files changed: 76 ins; 23 del; 5 mod
  Patch: https://git.openjdk.org/jdk/pull/21764.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/21764/head:pull/21764

PR: https://git.openjdk.org/jdk/pull/21764