8288948: Few J2DBench tests indicate lower primitive drawing performance with metal rendering pipeline

[aghaisas]

J2DBench test option files attached to JDK-8288948 indicate lower drawing performance on macOS with Metal rendering pipeline as compared to OpenGL rendering pipeline.

Analysis :
Current implementation of 2D primitives (Line, Rectangle, Parallelogram - Draw/Fill operations) in Metal rendering pipeline follow below structure-

1. End points (vertices) required for the primitive drawing are put in a buffer.
2. The data prepared in above step is sent to GPU using MTLRenderCommandEncoder setVertexBytes() call
3. A draw command is issued using MTLRenderCommandEncoder drawPrimitives() call
4. Primitive Color is set (repeated when encoder or color changes) using MTLRenderCommandEncoder setFragmentBytes() call in MTLRenderCommandEncoder state update.

Root Cause of slower performance :
It is found that the multiple calls to MTLRenderCommandEncoder drawPrimitives() by using MTLRenderCommandEncoder setVertexBytes() to send a tiny amount of data each time slows down the rendering.

Fix :
MTLRenderCommandEncoder setVertexBytes() can accept 4KB of buffer at a time.
The primitive drawing logic is modified to collate adjacent draw calls as below -

1. A buffer of size approximately equal to 4KB is created - this is treated as common vertex buffer which is reused again and again
2. For each primitive draw call - the vertices needed for that draw call are added to the above buffer
3. When the buffer is full OR some other condition occurs ( e.g. breakage of draw primitive sequence, some other operation as change of color etc) -
   a) Vertex data buffer is sent to the GPU using MTLRenderCommandEncoder setVertexBytes() call.
   b) A single (or multiple) draw command(s) are issued using MTLRenderCommandEncoder drawPrimitives() call.

More insight :
In general, an application requires a mix of 2D shapes, images and text of different color and sizes.
The performance test that we have measure rendering performance of extreme cases such as -

1. J2DBench - tests the repeated drawing of the same type and same color in a time period - e.g. Find the rendering speed of repeated 2D Line draw operation in X mSec?
2. RenderPerf test - tests the drawing of N primitives of the same type but each instance with a different color and capture FPS.

This PR optimizes the Java2D Metal rendering pipeline implementation for the first case where the same primitive is drawn repeatedly without changing its color. Our current architecture needs to be tweaked to address slower performance shown by RenderPerf tests. If needed, that needs to be done separately.

Results :
The performance results are attached to the JBS.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8288948: Few J2DBench tests indicate lower primitive drawing performance with metal rendering pipeline

Reviewers

• Alexey Ushakov (@avu - Committer)
• Phil Race (@prrace - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9245/head:pull/9245
$ git checkout pull/9245

Update a local copy of the PR:
$ git checkout pull/9245
$ git pull https://git.openjdk.org/jdk pull/9245/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9245

View PR using the GUI difftool:
$ git pr show -t 9245

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9245.diff

[bridgekeeper]

👋 Welcome back aghaisas! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@aghaisas The following label will be automatically applied to this pull request:

• client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (766ed69e)
• 00: Full (ea5e233f)

[avu]

Overal looks good

[avu]

Looks good

[victordyakov]

need a review from @prrace

[prrace]

Why are there several duplicates here ?

[aghaisas]

Earlier logic used to draw a rectangle with MTLPrimitiveTypeLineStrip. Hence, 5 vertices were specified to draw 4 sides of the rectangle.
Now, the logic has been changed to use MTLPrimitiveTypeLine. Hence, 4 lines need to be specified separately by specifying 8 vertices. There are duplicates since the lines are connected.

We cannot use MTLPrimitiveTypeLineStrip if we want to batch the subsequent draw calls as it draws an unwanted line between previous rectangle and current rectangle.

[mrserb]

We cannot use MTLPrimitiveTypeLineStrip if we want to batch the subsequent draw calls as it draws an unwanted line between previous rectangle and current rectangle.

Even if the alpha will be transparent? Just curious.

[aghaisas]

Yes, there will be a transparent line between previous rectangle and current rectangle.
This PR introduces batching of vertices of successive draw calls of the same primitive. If we keep on adding vertices to a common buffer and finally encode draw operation using MTLPrimitiveTypeLineStrip, all those vertices will be connected. If color is changed in between these successive draw calls, we end the current vertex batch and start a new one.

[mrserb]

But what about the usage of MTLPrimitiveTypeLineStrip and the transparent color to hide the "unwanted line between previous rectangle and current rectangle"? OR it is not possible to draw the lines using different colors in one step?

[aghaisas]

OR it is not possible to draw the lines using different colors in one step?

Exactly this is the point. If the color is changed (alpha change is also treated as color change) in between these successive draw calls, we end the current vertex batch and issue a draw command for accumulated vertices.
This is the reason MTLPrimitiveTypeLineStrip and MTLPrimitiveTriangleStrip cannot be used with our current vertex batching logic.

[openjdk]

@aghaisas This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8288948: Few J2DBench tests indicate lower primitive drawing performance with metal rendering pipeline

Reviewed-by: avu, prr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 314 new commits pushed to the master branch:

• 0184f46: 8289919: [test] LoadLibraryUnloadTest.java failed with "Failed to unload native library"
• cca91f7: 8290327: Remove java/lang/reflect/callerCache/ReflectionCallerCacheTest.java from ProblemList-Xcomp.txt
• b4e2ce0: 8290366: Remove unused during_conc_mark parameter in HeapRegion::note_self_forwarding_removal_start
• f3abb82: 8268312: Compilation error with nested generic functional interface
• 92deab5: 8028265: Add legacy tz tests to OpenJDK
• 70fce07: 8290246: test fails "assert(init != __null) failed: initialization not found"
• 757a742: 8290177: Improve documentation in G1MMUTracker
• 890bced: 8290264: java/util/concurrent/locks/Lock/OOMEInAQS.java fails with "exit code: 0"
• 3ad3950: Merge
• fd89ab8: 8288112: C2: Error: ShouldNotReachHere() in Type::typerr()
• ... and 304 more: https://git.openjdk.org/jdk/compare/d51f4f471f3941294a987dcb68ee264fe27f018a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[aghaisas]

/integrate

[openjdk]

Going to push as commit bc7a1ea.
Since your change was applied there have been 323 commits pushed to the master branch:

• 84f2314: 8286030: Avoid JVM crash when containers share the same /tmp dir
• 4dd236b: 8290280: riscv: Clean up stack and register handling in interpreter
• 522b657: Merge
• 15d3329: 8281969: Bad result for the snippet @link tag if substring/regex consists of whitespace
• c8e0315: 8290250: Shenandoah: disable Loom for iu mode
• fb27ddc: 8290252: Add TEST.properties to java/nio/channels/FileChannel and move tests out of largeMemory sub-dir
• 441c33f: 8289003: JavaThread::check_is_terminated() implementation should rely on Thread-SMR
• 2342684: 8290066: Remove KNL specific handling for new CPU target check in IR annotation
• 0143cf1: 8290333: Remove os_share_*.hpp
• 0184f46: 8289919: [test] LoadLibraryUnloadTest.java failed with "Failed to unload native library"
• ... and 313 more: https://git.openjdk.org/jdk/compare/d51f4f471f3941294a987dcb68ee264fe27f018a...master

Your commit was automatically rebased without conflicts.

[openjdk]

@aghaisas Pushed as commit bc7a1ea.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.