Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to TornadoNativeArray data types to get Memory Segments without header offset #350

Merged
merged 24 commits into from
Mar 19, 2024

Conversation

mikepapadim
Copy link
Member

@mikepapadim mikepapadim commented Mar 11, 2024

Description

This PR adds support to obtain a MemorySegment from TornadoNativeArray types with and without the header bytes.

  • MemorySegment getSegment() returns a memory segment excluding the Tornado header.
  • MemorySegment getSegmentWithHeader() returns a memory segment as allocated for TornadoVM including the Tornado header (24bytes).
  • long getNumBytesOfSegmentWithHeader() returns Segment's number of bytes including the Tornado header.
  • long getNumBytesOfSegment() returns Segment's number of bytes excluding the Tornado header.

By having distinct funtion in the above auxiliry methods we can have interoperoability when using Segments for GPU execution through the Taskgraph API and Vector API when using any of the TornadoNative types.

Also, refactor internally methods that operate on memory segments to be named explictly as getSegmentWithHeader.

Backend/s tested

Mark the backends affected by this PR.

  • OpenCL
  • PTX
  • SPIRV

OS tested

Mark the OS where this PR is tested.

  • Linux
  • OSx
  • Windows

Did you check on FPGAs?

If it is applicable, check your changes on FPGAs.

  • Yes
  • No

How to test the new patch?

make jdk21  BACKEND=openc
make tests 

Implemented a new method called getSegmentWithHeader in various array classes in tornado-api package. This method returns the underlying MemorySegment of the array instance, including the header offset. As a result, users can now access the whole segment which includes both the header and data, rather than data only.
The code has been updated to use "getSegmentWithHeader()" method instead of "getSegment()" across various OpenCL, SPIRV and PTX driver files. This adjustment ensures the appropriate header attached memory segment is used with operations like buffer reading, writing, allocation, etc. Reduces potential for errors in memory management and data handling.
@mikepapadim mikepapadim self-assigned this Mar 11, 2024
Revise SPIRVVectorWrapper to handle memory segments in a safer manner. Replaced usages of "getSegment()" with "getSegmentWithHeader()" for buffer actions such as enqueueReadBuffer, enqueueWriteBuffer, readBuffer, and writeBuffer. The change helps mitigate potential concurrency issues and ensures better safety in multithreaded environments.
Renamed methods related to array size calculations in TornadoNativeArray. The method getNumBytesOfSegment() is now getNumBytesOfSegmentWithHeader() and the method getNumBytesWithoutHeader() is now getNumBytesOfSegment(). All implementations and references to these methods have been updated accordingly across various classes.
The documentation has been updated to reflect changes in method names related to the off-heap array size calculations in TornadoNative
The error message for TornadoMemoryException in SPIRVVectorWrapper has been updated to dynamically display the bufferSize. This minor modification can improve debugging by indicating the exact size that caused the exception.
Copy link
Collaborator

@stratika stratika left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that in many of the classes, the year in the copyright header has not been updated to 2024.

@@ -31,4 +31,6 @@ public sealed interface TornadoImagesInterface<T extends Buffer> //

MemorySegment getSegment();

MemorySegment getSegmentWithHeader();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getNumBytes returns always the number of bytes with the header, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone will use the getNumBytes() for the segment returned via getSegment, will it work?

mikepapadim and others added 10 commits March 13, 2024 12:16
…rrays/TornadoNativeArray.java

Co-authored-by: Thanos Stratikopoulos <34061419+stratika@users.noreply.github.com>
…rrays/TornadoNativeArray.java

Co-authored-by: Thanos Stratikopoulos <34061419+stratika@users.noreply.github.com>
…rrays/ByteArray.java

Co-authored-by: Thanos Stratikopoulos <34061419+stratika@users.noreply.github.com>
…rrays/CharArray.java

Co-authored-by: Thanos Stratikopoulos <34061419+stratika@users.noreply.github.com>
…rrays/DoubleArray.java

Co-authored-by: Thanos Stratikopoulos <34061419+stratika@users.noreply.github.com>
…rrays/FloatArray.java

Co-authored-by: Thanos Stratikopoulos <34061419+stratika@users.noreply.github.com>
…rrays/HalfFloatArray.java

Co-authored-by: Thanos Stratikopoulos <34061419+stratika@users.noreply.github.com>
…rrays/ShortArray.java

Co-authored-by: Thanos Stratikopoulos <34061419+stratika@users.noreply.github.com>
…rrays/IntArray.java

Co-authored-by: Thanos Stratikopoulos <34061419+stratika@users.noreply.github.com>
…rrays/LongArray.java

Co-authored-by: Thanos Stratikopoulos <34061419+stratika@users.noreply.github.com>
@mikepapadim mikepapadim changed the title Add support to TornadoNativeArray data types to get Memory Segments without base offset Add support to TornadoNativeArray data types to get Memory Segments without header offset Mar 13, 2024
@jjfumero
Copy link
Member

I am confused. The withHeader version, does it always return +24 bytes, or it returns the bytes specified in TornadoNativeArray.ARRAY_HEADER?

@mikepapadim
Copy link
Member Author

I am confused. The withHeader version, does it always return +24 bytes, or it returns the bytes specified in TornadoNativeArray.ARRAY_HEADER?

withHeader returns always number of bytes including the TornadoNativeArray.ARRAY_HEADER

@jjfumero
Copy link
Member

I am confused. The withHeader version, does it always return +24 bytes, or it returns the bytes specified in TornadoNativeArray.ARRAY_HEADER?

withHeader returns always number of bytes including the TornadoNativeArray.ARRAY_HEADER

So please, update the PR description with this.

@mikepapadim
Copy link
Member Author

I am confused. The withHeader version, does it always return +24 bytes, or it returns the bytes specified in TornadoNativeArray.ARRAY_HEADER?

withHeader returns always number of bytes including the TornadoNativeArray.ARRAY_HEADER

So please, update the PR description with this.

done

docs/source/offheap-types.rst Outdated Show resolved Hide resolved
docs/source/offheap-types.rst Outdated Show resolved Hide resolved
@@ -29,23 +29,18 @@
public class DataRange {

private final TornadoNativeArray tornadoNativeArray;
private final long totalSizeInBytes;
private final int elementSize;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, if we all use the same IntelliJ templates for code formatting, why is gets shuffled every time?

@jjfumero
Copy link
Member

Unittests are passing on OSx 14.4

@mikepapadim
Copy link
Member Author

@jjfumero done

@jjfumero
Copy link
Member

ok, thanks. the PR looks good to me.

@mikepapadim mikepapadim merged commit 26691bb into beehive-lab:develop Mar 19, 2024
2 checks passed
@mikepapadim mikepapadim deleted the mikepapadim/segment_slice branch March 19, 2024 08:57
jjfumero added a commit to jjfumero/TornadoVM that referenced this pull request Mar 27, 2024
Improvements
~~~~~~~~~~~~~~~~~~

- `beehive-lab#344 <https://github.com/beehive-lab/TornadoVM/pull/344>`_: Support for Multi-threaded Execution Plans.
- `beehive-lab#347 <https://github.com/beehive-lab/TornadoVM/pull/347>`_: Enhanced examples.
- `beehive-lab#350 <https://github.com/beehive-lab/TornadoVM/pull/350>`_: Obtain internal memory segment for the Tornado Native Arrays without the object header.
- `beehive-lab#357 <https://github.com/beehive-lab/TornadoVM/pull/357>`_: API extensions to query and apply filters to backends and devices from the ``TornadoExecutionPlan``.
- `beehive-lab#359 <https://github.com/beehive-lab/TornadoVM/pull/359>`_: Support Factory Methods for FFI-based array collections to be used/composed in TornadoVM Task-Graphs.

Compatibility
~~~~~~~~~~~~~~~~~~

- `beehive-lab#351 <https://github.com/beehive-lab/TornadoVM/pull/351>`_: Compatibility of TornadoVM Native Arrays with the Java Vector API.
- `beehive-lab#352 <https://github.com/beehive-lab/TornadoVM/pull/352>`_: Refactor memory limit to take into account primitive types and wrappers.
- `beehive-lab#354 <https://github.com/beehive-lab/TornadoVM/pull/354>`_: Add DFT-sample benchmark in FP32.
- `beehive-lab#356 <https://github.com/beehive-lab/TornadoVM/pull/356>`_: Initial support for Windows 11 using Visual Studio Development tools.
- `beehive-lab#361 <https://github.com/beehive-lab/TornadoVM/pull/361>`_: Compatibility with the SPIR-V toolkit v0.0.4.
- `beehive-lab#366 <https://github.com/beehive-lab/TornadoVM/pull/363>`_: Level Zero JNI Dependency updated to 0.1.3.

Bug Fixes
~~~~~~~~~~~~~~~~~~

- `beehive-lab#346 <https://github.com/beehive-lab/TornadoVM/pull/346>`_: Computation of local-work group sizes for the Level Zero/SPIR-V backend fixed.
- `beehive-lab#360 <https://github.com/beehive-lab/TornadoVM/pull/358>`_: Fix native tests to check the JIT compiler for each backend.
- `beehive-lab#355 <https://github.com/beehive-lab/TornadoVM/pull/355>`_: Fix custom exceptions when a driver/device is not found.
@jjfumero jjfumero mentioned this pull request Mar 27, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants