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

8264064: Cross compile JavaFX graphics and controls modules for Windows AArch64 (ARM64) #439

Closed

Conversation

@AlexanderScherbatiy
Copy link

@AlexanderScherbatiy AlexanderScherbatiy commented Mar 23, 2021

This is a proposal for cross compiling JavaFX base modules (excluding media and webkit) for Windows AArch64 (ARM64).

Main changes:

  • prismES2 native compilation is moved under IS_INCLUDE_ES2 condition
  • HOST_ARCH and TARGET_ARCH are retrieved from ext.OS_ARCH and ext.TARGET_ARCH using substitution aarch64 to arm64 and amd64 to x64
  • VCARCH is set to "${HOST_ARCH}_${TARGET_ARCH}" architecture for cross compilation. VCARCH is set to x64 for amd64 target architecture (according to the vcvarsall.bat doc x64 and amd64 are interchangeable)
  • arm64 rc.exe and fxc.exe execution fails on non arm64 host. The fix checks that and falls back to host rc.exe and fxc.exe. The right way would be to use rc.exe and fxc.exe from arm64 but I did not find a right way to run them on host system.

I also looked which changes are required to build media module for Windows aarch64.
The main changes could be using:

  • ARCH=arm64 for building media libs in build.gradle file
  • -MACHINE:arm64 LDFLAGS in media make files
  • msvc_build/aarch64/aarch64_include header files for include, src/aarch64/win64_armasm.S for ASM_SOURCES, armasm64.exe for ML in gstreamer/projects/win/glib-lite/Makefile.glib file and corresponding include in gstreamer/projects/win/glib-lite/Makefile.gobject file
  • setting ENABLE_SIMD_SSE2 to 0 in ColorConverter.c in the similar way how it is done for Apple Silicon port

In this way the media is build but it is crashed when I run a JavaFX sample with video.
Is it possible to send the media Windows aarch64 port to review and investigate the crash in the separate fix?


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8264064: Cross compile JavaFX graphics and controls modules for Windows AArch64 (ARM64)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/439/head:pull/439
$ git checkout pull/439

Update a local copy of the PR:
$ git checkout pull/439
$ git pull https://git.openjdk.java.net/jfx pull/439/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 439

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/439.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 23, 2021

👋 Welcome back alexsch! 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 openjdk bot added the rfr label Mar 23, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 23, 2021

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Mar 23, 2021

/reviewers 2

@kevinrushforth kevinrushforth self-requested a review Mar 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 23, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@AlexanderScherbatiy
Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy commented Apr 13, 2021

I removed ext.MC = cygpath("$winSdkBinDir/mt.exe") line from win.gradle because it was used for building jdk.packager module openjfx/11-dev/rt/build.gradle and now jdk.packager is removed from jfx.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

I think this is is fine. I did a local build to ensure that x64 builds are not impacted.

I left a question and a comment inline.

buildSrc/win.gradle Show resolved Hide resolved
switch (arch) {
case "aarch64" : return "arm64"
case "amd64" : return "x64"
default: return arch
Copy link
Member

@kevinrushforth kevinrushforth Apr 13, 2021

Choose a reason for hiding this comment

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

This looks fine. I recommend to sanity test this on a 32-bit build to ensure that os.arch is x86 (I don't have the ability to do that).

Choose a reason for hiding this comment

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

Here are some println lines which I added to win.gradle file before building the fix for windows 32 bits

println "CPU_BITS: $CPU_BITS, ext.OS_ARCH: ${ext.OS_ARCH}"
println "HOST_ARCH: $HOST_ARCH, TARGET_ARCH: ${TARGET_ARCH}, IS_CROSS: ${IS_CROSS}"

I built the fix on Windows x86_64 with 32bits java
The output was:

CPU_BITS: x86, ext.OS_ARCH: x86
HOST_ARCH: x86, TARGET_ARCH: x86, IS_CROSS: false

file showed that the glass.dll has been build for Intel 80386:

file build/sdk/bin/glass.dll 
build/sdk/bin/glass.dll: PE32 executable (DLL) (GUI) Intel 80386, for MS Windows

I built corresponding jdk 16 with --with-target-bits=32 option and the jfx and was able to successfully run Ensemble8.jar app on Windows 32bits.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good.

buildSrc/win.gradle Show resolved Hide resolved
@kevinrushforth kevinrushforth requested a review from johanvos Apr 14, 2021
@@ -41,6 +41,10 @@ WIN.modLibDest = "lib"

def CPU_BITS = IS_64 ? "x64" : "x86"

def HOST_ARCH = getWinArch(ext.OS_ARCH)
def TARGET_ARCH = getWinArch(ext.TARGET_ARCH)
def IS_CROSS = HOST_ARCH != TARGET_ARCH
Copy link
Collaborator

@johanvos johanvos Apr 14, 2021

Choose a reason for hiding this comment

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

can we move this to build.gradle?
With the increasing number of supported configurations, there will otherwise be more gradle sub-files that have something similar.

Copy link
Member

@kevinrushforth kevinrushforth Apr 14, 2021

Choose a reason for hiding this comment

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

Yes, this does seem like a better plan. Should this be done as a follow-on or do you want to see it done now? One reason I ask is that my PR #462 (which is now approved, but waiting re-review) does something similar to getWinArch() to determine whether we are running on aarch64 or not and should be modified as well.

Copy link
Collaborator

@johanvos johanvos Apr 14, 2021

Choose a reason for hiding this comment

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

In that case, a follow-up seems best (we can tackle mac/win/linux aarch64 simultaneously then)

Choose a reason for hiding this comment

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

I added CONVERTED_OS_ARCH, CONVERTED_TARGET_ARCH variables, and getConvertedOsArch(arch) function to build.gradle.

I think there should be the better prefix name for updated os arch but what comes in my mind is converted or unified.

Building jfx with the fix on windows 32, 64 bits and arm64 cross compilation outputs the following variables in the log:

# Win 32 bits
OS_ARCH: x86
TARGET_ARCH: x86
CONVERTED_OS_ARCH: x86
CONVERTED_TARGET_ARCH: x86

# Win 64 bits
OS_ARCH: amd64
TARGET_ARCH: amd64
CONVERTED_OS_ARCH: x64
CONVERTED_TARGET_ARCH: x64

# Win arm64 cross compilation
OS_ARCH: amd64
TARGET_ARCH: aarch64
CONVERTED_OS_ARCH: x64
CONVERTED_TARGET_ARCH: arm64

build.gradle Outdated
case "aarch64" : return "arm64"
case "amd64" : return "x64"
default: return arch
}
Copy link
Collaborator

@johanvos johanvos Apr 15, 2021

Choose a reason for hiding this comment

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

I think a function that gets the "converted" arch is very useful, but I'm not sure what the resulting value should be. In case of arm64/aarch64, I have s slight preference for aarch64, but I'm not against arm64.

Copy link
Member

@kevinrushforth kevinrushforth Apr 15, 2021

Choose a reason for hiding this comment

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

This does seem useful, although in this case, wasn't arm64 chosen for compatibility with the name in the Microsoft VisualStudio distro? If so, then this part of the most recent change might be Windows-specific. It might be better to revert this (going back to your previous fix in win.gradle) until we can take a more holistic look at this.

Choose a reason for hiding this comment

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

Yes, the arm64 is used as an OS arch for Visual Studio vcvarsall.bat and x64 as a part of a path to arch specific Visual Studio tools on Windows.

I reverted the fix with converted os arch variables back.

@kevinrushforth kevinrushforth self-requested a review Apr 15, 2021
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Thanks.

Let's file a follow-on bug to refactor the parts of the logic that can be made platform independent, and also to rationalize aarch64 vs arm64 (the latter is needed on both Windows and macOS for some paths or compiler options, but the former is what we would like to use in the build logic for the name of the platform where we can).

@johanvos
Copy link
Collaborator

@johanvos johanvos commented Apr 17, 2021

I'll do a test in our CI.

Copy link
Collaborator

@johanvos johanvos left a comment

Build works fine. Ultimately, we need to standardize on ARCH naming conventions in build.gradle and this will impact the usage of ${HOST_ARCH}_${TARGET_ARCH} but it looks good the way it is currently implemented.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 20, 2021

@AlexanderScherbatiy 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:

8264064: Cross compile JavaFX graphics and controls modules for Windows AArch64 (ARM64)

Reviewed-by: kcr, jvos

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 28 new commits pushed to the master branch:

  • 86b854d: 8234077: Evaluate ignored unit tests in RenderRootTest
  • 8f0062d: 8234084: [TEST_BUG] Remove unexecuted performance benchmark tests
  • 49d2126: 8263788: JavaFX application freezes completely after some time when using the WebView
  • 67828ae: 8261840: Submenus close to screen borders are no longer repositioned
  • af75a1f: 8265439: [TestBug] Enable and fix ignored unit tests in MenuItemTest
  • e02cee9: 8264990: WebEngine crashes with segfault when not loaded through system classloader
  • a683558: 8259356: MediaPlayer's seek freezes video
  • 56f2e17: 8088739: [TestBug] RegionBackgroundImageUITest fail with timeout error
  • 8e54757: 8089589: [ListView] ScrollBar content moves toward-backward during scrolling.
  • 05ab799: 8208088: Memory Leak in ControlAcceleratorSupport
  • ... and 18 more: https://git.openjdk.java.net/jfx/compare/d4d57fb177e0bda717fb3b77901d54081130561b...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @johanvos) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Apr 20, 2021
@johanvos
Copy link
Collaborator

@johanvos johanvos commented Apr 21, 2021

@AlexanderScherbatiy I think this is good to get in now, can you /integrate it?

@AlexanderScherbatiy
Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy commented Apr 22, 2021

/integrate

@openjdk openjdk bot added the sponsor label Apr 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 22, 2021

@AlexanderScherbatiy
Your change (at version b898bb4) is now ready to be sponsored by a Committer.

@johanvos
Copy link
Collaborator

@johanvos johanvos commented Apr 22, 2021

/sponsor

@openjdk openjdk bot closed this Apr 22, 2021
@openjdk openjdk bot added integrated and removed sponsor labels Apr 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 22, 2021

@johanvos @AlexanderScherbatiy Since your change was applied there have been 28 commits pushed to the master branch:

  • 86b854d: 8234077: Evaluate ignored unit tests in RenderRootTest
  • 8f0062d: 8234084: [TEST_BUG] Remove unexecuted performance benchmark tests
  • 49d2126: 8263788: JavaFX application freezes completely after some time when using the WebView
  • 67828ae: 8261840: Submenus close to screen borders are no longer repositioned
  • af75a1f: 8265439: [TestBug] Enable and fix ignored unit tests in MenuItemTest
  • e02cee9: 8264990: WebEngine crashes with segfault when not loaded through system classloader
  • a683558: 8259356: MediaPlayer's seek freezes video
  • 56f2e17: 8088739: [TestBug] RegionBackgroundImageUITest fail with timeout error
  • 8e54757: 8089589: [ListView] ScrollBar content moves toward-backward during scrolling.
  • 05ab799: 8208088: Memory Leak in ControlAcceleratorSupport
  • ... and 18 more: https://git.openjdk.java.net/jfx/compare/d4d57fb177e0bda717fb3b77901d54081130561b...master

Your commit was automatically rebased without conflicts.

Pushed as commit 4a9c892.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants