[foreign-memaccess+abi] RFR: 8292034: Improve javadoc after memory segment/memory address unification [v2]

[Maurizio Cimadamore]

On Fri, 23 Sep 2022 11:49:34 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Replace "alignment constraints" with "alignment constraint", in accordance to what done in `MemorySegment` javadoc
>
> src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 91:
> 
>> 89:  * A heap segment obtained from one of the {@link #ofArray(int[])} factory methods has an address of zero.</li>
>> 90:  * <li>The address of a native segment (including mapped segments) denotes the physical address of the region of
>> 91:  * memory which backs the segment.</li>
> 
> I wonder if "physical address" could be confusing here. Since, a process in the operating system is typically assigned a virtual address space that maps to physical memory, and it's the virtual addresses we expose.
> 
> Maybe:
> Suggestion:
> 
>  * <li>The address of a native segment (including mapped segments) denotes the address in native memory of the region of
>  * memory which backs the segment.</li>

I get your concern - however, by physical here we refer to the address that e.g. a C/C++ program would see. The fact that physical addresses are virtualized by the OS is a concern we gloss over here - honestly I don't think this is a big deal?

> src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 201:
> 
>> 199:  * Access operations on a memory segment are constrained not only by the spatial and temporal bounds of the segment,
>> 200:  * but also by the <em>alignment constraint</em> of the value layout specified to the operation. An access operation can
>> 201:  * access only those offsets in the segment that denote addresses in physical memory which are <em>aligned</em> according
> 
> Again, "physical" might be confusing here. I suggest maybe just dropping it.
> 
> Suggestion:
> 
>  * access only those offsets in the segment that denote addresses in memory which are <em>aligned</em> according

I note here that we use physical address, but here we say address in physical memory - which seems like a mismatch (we never defined the term physical memory).

> src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 214:
> 
>> 212:  * <p>
>> 213:  * If the segment being accessed is a native segment, then its {@linkplain #address() address} in physical memory can be
>> 214:  * combined with the offset to obtain the <em>target address</em> in physical memory. The pseudo-function below demonstrates this:
> 
> Suggestion:
> 
>  * If the segment being accessed is a native segment, then its {@linkplain #address() address} can be
>  * combined with the offset to obtain the <em>target address</em> in native memory. The pseudo-function below demonstrates this:

Ok, it seems there's a lot of those.

> src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 224:
> 
>> 222:  * For example:
>> 223:  * <ul>
>> 224:  * <li>A native segment at address 1000 can be accessed at offsets 0, 8, 16, 24, etc under an 8-byte alignment constraint,
> 
> I suggest maybe changing 'at address' -> 'with address' here, and below. Since 'native segment' seems to refer to the Java object, and it is the backing memory region that is _at_ a certain address.

Agree - the address is a property of the segment

> src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 245:
> 
>> 243:  * </ul>
>> 244:  * <!-- Under a 1-byte alignment constraint, any offset address in a native segment can be accessed. -->
>> 245:  * <!-- In effect, every target address is at least 1-byte aligned. -->
> 
> This seems to be an html comment, and doesn't show up in the rendered javadoc. Not sure if that's intentional?

whoops - that should be removed.

> src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 943:
> 
>> 941:      *
>> 942:      * @param buffer the buffer instance to be wrapped by a new memory segment.
>> 943:      * @return a memory segment, wrapping the given buffer instance.
> 
> Since there's not always a wrapping happening, maybe:
> Suggestion:
> 
>      * @param buffer the buffer instance to be turned into a new memory segment.
>      * @return a memory segment, derived from the given buffer instance.

Good suggestion, we've been generally trying to avoid "wrapping"

-------------

PR: https://git.openjdk.org/panama-foreign/pull/730