Skip to content
This repository has been archived by the owner. It is now read-only.

8258754: Gracefully fallback to the OpenGL rendering pipeline if Metal rendering pipeline initialization fails #147

Closed
wants to merge 10 commits into from

Conversation

@aghaisas
Copy link
Collaborator

@aghaisas aghaisas commented Jan 6, 2021

This implements fallback to the OpenGL rendering pipeline if Metal rendering pipeline initialization fails.
I have tested this fallback on 10.15.4 by stubbing code. We need real testing on a system where Metal is not available.


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8258754: Gracefully fallback to the OpenGL rendering pipeline if Metal rendering pipeline initialization fails

Reviewers

Download

$ git fetch https://git.openjdk.java.net/lanai pull/147/head:pull/147
$ git checkout pull/147

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 6, 2021

👋 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
Copy link

@openjdk openjdk bot commented Jan 6, 2021

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

8258754: Gracefully fallback to the OpenGL rendering pipeline if Metal rendering pipeline initialization fails

Reviewed-by: kcr, 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 46 new commits pushed to the master branch:

  • ffff4e1: 8259853: Lanai: nonAA Gradient painting is not precise for VI
  • 61b650c: Automatic merge of jdk:master into master
  • 533a2d3: 8258961: move some fields of SafePointNode from public to protected
  • 6b4732f: 8259679: GitHub actions should use MSVC 14.28
  • 061ffc4: 8249245: assert(((((JfrTraceIdBits::load(klass)) & ((JfrTraceIdEpoch::this_epoch_method_and_class_bits()))) != 0))) failed: invariant
  • e60c992: 8259849: Shenandoah: Rename store-val to IU-barrier
  • db9c114: 7146776: deadlock between URLStreamHandler.getHostAddress and file.Handler.openconnection
  • 61292be: 8259681: Remove the Marlin rendering engine (single-precision)
  • ff275b3: 8259403: Zero: crash with NULL MethodHandle receiver
  • e93f08e: 8074101: Add verification that all tasks are actually claimed during roots processing
  • ... and 36 more: https://git.openjdk.java.net/lanai/compare/98822bb3256ec61b5eb3dee1d88604a7b834121f...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.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 6, 2021

@@ -503,7 +502,7 @@ public Insets getScreenInsets(final GraphicsConfiguration gc) {
@Override
public void sync() {
// flush the OGL pipeline (this is a no-op if OGL is not enabled)
if (MacOSFlags.isMetalEnabled()) {
if (CGraphicsDevice.useMetalPipeline()) {
MTLRenderQueue.sync();

This comment has been minimized.

@mrserb

mrserb Jan 7, 2021
Member

Does this check mean that the MTLRenderQueue.sync() is not a noop if the metal is not enabled?

This comment has been minimized.

@aghaisas

aghaisas Jan 7, 2021
Author Collaborator

Does this check mean that the MTLRenderQueue.sync() is not a noop if the metal is not enabled?

I don't know how else we will differentiate between Metal and OGL here.
MTLRenderQueue.sync() is still a noop - as there is null instance check inside it.
The comment above the if check has become stale - I will update it.

CGLGraphicsConfig.getConfig(this);

// Check whether -Dsun.java2d.metal=true has been specified
if (MacOSFlags.isMetalEnabled()) {

This comment has been minimized.

@mrserb

mrserb Jan 7, 2021
Member

Is it possible to use metal and OGL at the same time on different devices? I think it is better to move these checks to the CGraphicsEnvironment.makeScreenDevice(), similar Win32GraphicsEnvironment.makeScreenDevice()

This comment has been minimized.

@aghaisas

aghaisas Jan 7, 2021
Author Collaborator

I am not sure why someone wants to use different pipelines on different devices? and should we support it?
On Windows we do that as a fallback to overcome a limitation of WGL - that's what I could infer from code in W32GraphicsDevice.getDefaultConfiguration()

I doubt whether we can add these checks in CGraphicsEnvironment.makeScreenDevice() - as we do not have specific MTLGraphicsDevice and CGLGraphicsDevice classes. That's the reason this method is currently unsupported.

Win32GraphicsEnvironment.makeScreenDevice() - creates either D3DGraphicsDevice or Win32GraphicsDevice based on whether isD3DEnabled.
Win32GraphicsDevice in turn uses either WGLGraphicsConfig or Win32GraphicsConfig based on whether OGL is enabled.

I felt CGraphicsDevice (similar to Win32GraphicsDevice) can differentiate between metal or OGL. Hence, I have added checks here.

This comment has been minimized.

@mrserb

mrserb Jan 7, 2021
Member

If it is unsupported then why you should check that for every device creation? What happens if one device was created using metal and then another device will use OGL?

This comment has been minimized.

@aghaisas

aghaisas Jan 7, 2021
Author Collaborator

I have moved the Metal framework availability check to be done only once now.
There is still a check if MTLGraphicsConfig.getConfig() fails by any chance.

CGraphicsDevice contains GraphicsConfiguration - which is created in the constructor - It can be either MTLGraphicsConfiguration or OpenGLConfiguration. This check needs to be done while creating the config.
For Win32GraphicsDevice - the config is not created in the constructor, but at a later stage in getDefaultConfiguration(). So only the place of checking for OGL or falling back to GDI is different, but the concept is the same.

The whole idea of one device using metal and another device using OGL is hypothetical. The condition check cannot pass for one device and fail for another.

This comment has been minimized.

@kevinrushforth

kevinrushforth Jan 7, 2021
Member

I agree that it makes no sense to even consider using different pipelines on different devices. The change you made to do the check once seems best.

@jayathirthrao
Copy link
Member

@jayathirthrao jayathirthrao commented Jan 8, 2021

My 2 cents. Me and Ajit discussed this internally adding this info in PR also.

  1. No options - Use OpenGL(Switch to Metal later when we want to make it default)
  2. opengl == true || metal == false - Use OpenGL
  3. metal initialization fails print log in verbose- Use OpenGL
  4. metal == true || opengl == false - Use Metal
  5. opengl initialization fails print log in verbose(In initial release when OpenGL is default, i am not 100% sure whether we should try Metal pipeline or just exit) - Use Metal / exit
  6. Both fails print log and exit

All these things can be handled in this PR or future changes.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 8, 2021

Mailing list message from Prasanta Sadhukhan on lanai-dev:

On 08-Jan-21 1:33 PM, Jayathirth D V wrote:

On Wed, 6 Jan 2021 11:05:56 GMT, Ajit Ghaisas <aghaisas at openjdk.org> wrote:

This implements fallback to the OpenGL rendering pipeline if Metal rendering pipeline initialization fails.
I have tested this fallback on 10.15.4 by stubbing code. We need real testing on a system where Metal is not available.
My 2 cents. Me and Ajit discussed this internally adding this info in PR also.

1) No options - Use OpenGL(Switch to Metal later when we want to make it default)
2) opengl == true || metal == false - Use OpenGL
3) metal initialization fails print log in verbose- Use OpenGL

Maybe we can make this check version specific (maybe 10.12 for now) so
that in future version when opengl is removed, then this check will be
noop and if Metal fails, then it will quit.

Regards
Prasanta

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 8, 2021

  1. No options - Use OpenGL(Switch to Metal later when we want to make it default)

  2. opengl == true || metal == false - Use OpenGL

  3. metal initialization fails print log in verbose- Use OpenGL

  4. metal == true || opengl == false - Use Metal

  5. opengl initialization fails print log in verbose(In initial release when OpenGL is default, i am not 100% sure whether we should try Metal pipeline or just exit) - Use Metal / exit

  6. Both fails print log and exit

Unless I am misreading what you wrote, I think you have 3 and 5 switched. I would write it a bit differently in any case.

  • No options - use the default pipeline (OpenGL today, Metal later), fallback to the other pipeline (Metal today, OpenGL later) if the default pipeline init fails

This is the most important case, since it is what end users will typically see (i.e., setting sun.java2d.metal is a developer option that wouldn't typically be set in production). In the absence of any flags we must fallback to the non-default pipeline if initializing the default pipeline fails.

  • opengl == true - Use OpenGL, fallback to metal if OpenGL init fails

  • metal == true - Use Metal, fallback to OpenGL if Metal init fails

  • In all cases above, if both pipelines fail to initialize, then log error and throw an exception (a library should never call exit).

Btw, if both are set (once an "OpenGL" flag is added for the Mac), the precedence would be decided by whichever you check first -- probably metal, but it really doesn't matter as long as it is clearly spelled out.

@aghaisas aghaisas closed this Jan 8, 2021
@aghaisas aghaisas reopened this Jan 8, 2021
@aghaisas
Copy link
Collaborator Author

@aghaisas aghaisas commented Jan 8, 2021

Maybe we can make this check version specific (maybe 10.12 for now) so
that in future version when opengl is removed, then this check will be
noop and if Metal fails, then it will quit.

Regards
Prasanta

It is better to avoid version specific check when we can cover all the combinations discussed in other comments. My last commit addresses it.

@aghaisas
Copy link
Collaborator Author

@aghaisas aghaisas commented Jan 8, 2021

In the absence of specification, I have implemented the below combinations -

Current hard-coded Default pipeline is : OpenGL
If user specifies : Nothing --> Selected pipeline is : Default
If user specifies : opengl=true --> Selected pipeline is : OpenGL
If user specifies : opengl=false --> Selected pipeline is : Metal
If user specifies : metal=true --> Selected pipeline is : Metal
If user specifies : metal=false --> Selected pipeline is : OpenGL
If user specifies : opengl=true AND metal=false --> Selected pipeline is : OpenGL
If user specifies : opengl=false AND metal=true --> Selected pipeline is : Metal
If user specifies : opengl=true AND metal=true --> Selected pipeline is : Default
If user specifies : opengl=false AND metal=false --> Selected pipeline is : Default

Note that - I have not considered the order of VM options. If needed, that should be handled separately later.

If selected pipeline could not be initialized, then the other pipeline is used as fallback.
For fallback, appropriate message is logged only in verbose mode.
If fallback also fails (unlikely) - then a RuntimeException with appropriate message is thrown.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 8, 2021

Yes, I think this is what we what.

Note that - I have not considered the order of VM options. If needed, that should be handled separately later.

I don't think this is possible (even if it were desired).

Copy link
Member

@kevinrushforth kevinrushforth left a comment

The logic looks correct to me (I didn't run it). I did leave one question about whether we want to initialize both pipelines in the typical case where we don't need a fallback.

@prrace
Copy link
Collaborator

@prrace prrace commented Jan 8, 2021

Yes, I think this is what we what.

Note that - I have not considered the order of VM options. If needed, that should be handled separately later.

I don't think this is possible (even if it were desired).

I am sure it is not.
I think what we can do, which is really all we can do, is be consistent in the order in which we consider the pipelines.

I think the code we write here for mac ought to be written in a way that when we flip
the switch to make metal peferred / default that we don't have a lot of code to move around.

In as few words as possible :
The default pipeline will be used so long as it is supported, and in the absence of any other
unambiguous request for another supported pipeline.

It is expected that some pipeline is available, so long as the windowing system is running and reachable.
In such an unlikely event no pipeline is supported an InternalError will be thrown.

unambiguous means that with opengl as default

  • if you ask for both, we first try opengl.
  • if you disable both, we first try opengl
  • if you ask for neither, we first try opengl
  • if you don't enable some other pipeline, we first try opengl

when we flip the switch to metal as default then :

  • if you ask for both, we first try metal.
  • if you disable both, we first try metal
  • if you ask for neither, we first try metal
  • if you don't enable some other pipeline, we first try metal
@prrace
Copy link
Collaborator

@prrace prrace commented Jan 8, 2021

this implies that -Dsun.java2d.opengl=false on its own means the same as -Dsun.java2d.metal=true.

This is a bit grayer. It would imply that there is a designated fall back.

I take that back - it isn't grey. Because the default for metal is false, it means you've disabled both so rule 1 applies
I'll add another rule up above to make it clearer.

But .. we have a sort of precedent on windows that d3d=false means fall back to gdi. So that contradicts the above :-(

So is metal a "fallback" or an "option" ? We don't fall back to optional pipelines. Meaning d3d=false would not cause opengl to be used on windows.

@aghaisas
Copy link
Collaborator Author

@aghaisas aghaisas commented Jan 11, 2021

this implies that -Dsun.java2d.opengl=false on its own means the same as -Dsun.java2d.metal=true.

This is a bit grayer. It would imply that there is a designated fall back.

I take that back - it isn't grey. Because the default for metal is false, it means you've disabled both so rule 1 applies
I'll add another rule up above to make it clearer.

But .. we have a sort of precedent on windows that d3d=false means fall back to gdi. So that contradicts the above :-(

So is metal a "fallback" or an "option" ? We don't fall back to optional pipelines. Meaning d3d=false would not cause opengl to be used on windows.

My thinking differs on this. When we will integrate Lanai JEP to the mainline, we will have 2 rendering pipelines on macOS - OpenGL and Metal - OpenGL being default.
If user specifically asks for -Dsun.java2d.opengl=false, then we should definitely try to use the other available pipeline - whether it is fallback or optional is immaterial. Otherwise, we would end up in a situation where no UI will be launched.

I suggest that we deliberate and decide on this outside of this PR.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 11, 2021

Mailing list message from Alexey Ushakov on lanai-dev:

On 7 Jan 2021, at 10:28, Ajit Ghaisas <aghaisas at openjdk.java.net> wrote:

On Thu, 7 Jan 2021 06:32:59 GMT, Sergey Bylokhov <serb at openjdk.org> wrote:

Does this check mean that the MTLRenderQueue.sync() is not a noop if the metal is not enabled?

I don't know how else we will differentiate between Metal and OGL here.
MTLRenderQueue.sync() is still a noop - as there is null instance check inside it.
The comment above the if check has become stale - I will update it.

src/java.desktop/macosx/classes/sun/awt/CGraphicsDevice.java line 71:

69:
70: // Check whether -Dsun.java2d.metal=true has been specified
71: if (MacOSFlags.isMetalEnabled()) {

Is it possible to use metal and OGL at the same time on different devices? I think it is better to move these checks to the CGraphicsEnvironment.makeScreenDevice(), similar Win32GraphicsEnvironment.makeScreenDevice()

I am not sure why someone wants to use different pipelines on different devices? and should we support it?
On Windows we do that as a fallback to overcome a limitation of WGL - that's what I could infer from code in W32GraphicsDevice.getDefaultConfiguration()

I also think that it?s not a good idea to have both OGL and Metal enabled at the same time. It seems like it?s not a common scenario for client apps, so we may have some problems that Apple doesn?t even know about. Anyway OGL is now deprecated technology on Mac.

Best Regards,
Alexey

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 11, 2021

Mailing list message from Alexey Ushakov on lanai-dev:

Hi Ajit,

Let?s have some refactoring here.

We have two places this:

107 surfaceData = (CGraphicsDevice.usingMetalPipeline()) ?

108 ((MTLLayer)windowLayer).replaceSurfaceData() :
109 ((CGLLayer)windowLayer).replaceSurfaceData()
110 ;
111 return surfaceData;
112 }

I think it would be better to have a common superclass for CGLayer and MTLLayer with common methods replaceSurfaceData(), getPointer().

public class CFLayer extends CFRetainedResource {
?
public long getWindowLayerPtr() {}

public SurfaceData replaceSurfaceData() {}
...
}

So, we can get rid of (CGraphicsDevice.usingMetalPipeline()) ? ? : ? checks

Best Regards,
Alexey

@aghaisas
Copy link
Collaborator Author

@aghaisas aghaisas commented Jan 11, 2021

Mailing list message from Alexey Ushakov on lanai-dev:

Hi Ajit,

Let?s have some refactoring here.

We have two places this:

107 surfaceData = (CGraphicsDevice.usingMetalPipeline()) ?

108 ((MTLLayer)windowLayer).replaceSurfaceData() :
109 ((CGLLayer)windowLayer).replaceSurfaceData()
110 ;
111 return surfaceData;
112 }

I think it would be better to have a common superclass for CGLayer and MTLLayer with common methods replaceSurfaceData(), getPointer().

public class CFLayer extends CFRetainedResource {
?
public long getWindowLayerPtr() {}

public SurfaceData replaceSurfaceData() {}
...
}

So, we can get rid of (CGraphicsDevice.usingMetalPipeline()) ? ? : ? checks

Best Regards,
Alexey

Thanks Alexey for this suggestion.
Yes. It will simplify the code. This PR is about implementing the fallback mechanism. Once that is in place, we can handle the runtime pipeline selection pattern correction under JDK-8226384.

FILE *f = popen("/usr/sbin/system_profiler SPDisplaysDataType", "r");
bool metalSupport = FALSE;
bool metalSupported = JNI_FALSE;
while (getc(f) != EOF)
{

This comment has been minimized.

@prrace

prrace Jan 14, 2021
Collaborator

This strikes me as a slow way to find if metal is supported and I am not sure it is what I would call a "supported interface" either. Isn't there a better, more robust, faster way ?
Even if we don't change it today, we should file a bug on this to remember.

This comment has been minimized.

@aghaisas

aghaisas Jan 15, 2021
Author Collaborator

As of now, there is no known Metal API to do it in a better way. I have created JDK-8259825 to address it later.

metalEnabled = getBooleanProp("sun.java2d.metal", false);
PropertyState oglState = getBooleanProp("sun.java2d.opengl", PropertyState.UNSPECIFIED);
PropertyState metalState = getBooleanProp("sun.java2d.metal", PropertyState.UNSPECIFIED);

This comment has been minimized.

@prrace

prrace Jan 14, 2021
Collaborator

Right here we could add
if (metalState == PropertyState.UNSPECIFIED && oglState == PropertyState.UNSPECIFIED) {
// change this line to set metalState to toggle the default pipeline.
oglState = PropertyState.ENABLED;
}
This makes it easier to set the default

This comment has been minimized.

@aghaisas

aghaisas Jan 15, 2021
Author Collaborator

Good suggestion.
We need to handle other invalid combinations here as well - I will update the code.

}
} else if (metalState == PropertyState.ENABLED) {
if (oglState == PropertyState.ENABLED) {
oglEnabled = true;

This comment has been minimized.

@prrace

prrace Jan 15, 2021
Collaborator

By adding the invalid state handling further up, this condition can never be true

This comment has been minimized.

@kevinrushforth

kevinrushforth Jan 19, 2021
Member

Right. By the time we get here, the two flags cannot possibly be the same value, so you could simplify this by removing this check. That could be done as a follow-up.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 15, 2021

Mailing list message from Sergey Bylokhov on lanai-dev:

On 15.01.2021 03:04, Ajit Ghaisas wrote:

As of now, there is no known Metal API to do it in a better way. I have created JDK-8259825 to address it later.

How it will work if we will try to create a metal surface/device/etc when the metal is unsupported, I guess it just fail and we already should handle that?

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 16, 2021

Mailing list message from Alan Snyder on lanai-dev:

I must admit I am surprised at how complex the solution is to a simple observation.

Specifically:

1. What is the point of having multiple boolean command line parameters? Instead of separately enabling and disabling pipelines, why not have a single parameter that lists the desired pipelines in priority order. If none of the listed pipelines are available, then the JDK should be free to make its own choice of an available pipeline.

2. I presume this issue is currently a macOS-specific issue, but couldn't a similar issue arise in the future on other platforms? That would argue for using a parameter whose name includes the platform name, so that instructions for multiple platforms (e.g. in some testing script) can be simultaneously specified.

Alan

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 16, 2021

Conceptually, having an ordered list of the (two) pipelines makes sense (it's what we did for JavaFX with the "prism.order" property).

For Java2D, in order to fit with existing pattern and existing property names used on other platforms, we will need to stick with the separate properties as defined in this PR.

Ultimately, Phil gets to decide this, but at a high level here is what I recommend (I think it's pretty much what Ajit's latest patch intends to do, although I haven't looked at the latest changes in enough detail to be sure that's what it does).

  1. If no properties are specified, try the default pipeline first and "fall back" to the optional pipeline if the default isn't available.

  2. If exactly one of sun.java2d.opengl or sun.java2d.metal is set to true, then try that pipeline first and the other one as a fallback.

  3. If exactly one of sun.java2d.opengl or sun.java2d.metal is set to false, then try the other one first and that one last.

  4. If both are set to true or both are set to false, print a warning and ignore the properties entirely (i.e., use the default).

The following pseudo-code illustrates this:

    // default pipeline order
    pipelineOrder = { OPENGL, METAL };

    if ("sun.java2d.opengl" and "sun.java2d.metal" are both set and are equal) {
        // WARN and use the above default
    } else if ("sun.java2d.opengl" is "true" || "sun.java2d.metal" is "false") {
        pipelineOrder = { OPENGL, METAL };
    } else if ("sun.java2d.opengl" is "false" || "sun.java2d.metal" is "true") {
            pipelineOrder = { METAL, OPENGL };
        }
    }

    // Try the first pipeline in the list; if it fails, try the second

One advantage of the above is that it is trivial to change the default pipeline by just reversing the order in the initial list with no other changes needed.

@aghaisas
Copy link
Collaborator Author

@aghaisas aghaisas commented Jan 18, 2021

_Mailing list message from Sergey Bylokhov on [lanai-dev](mailto:lanai-
How it will work if we will try to create a metal surface/device/etc when the metal is unsupported, I guess it just fail and we already should handle that?

Yes, we do handle conditions if metal device creation and metal shader library loading fails - but, I am not sure whether we get "symbol not found" error if we try to use JDK built on a system with metal framework and then download that bundle on a system where metal is not supported and test. Hence this additional check that uses system profiler command was introduced to detect Metal framework availability to bail out early if metal is not supported.
I have added a comment to JDK-8259825 to see whether we can get rid of this check altogether.

@aghaisas
Copy link
Collaborator Author

@aghaisas aghaisas commented Jan 18, 2021

Alan & @kevinrushforth,
The new VM option for ordering of the pipelines is a good suggestion, but it needs to be looked at more carefully as we will still need to support existing VM options for rendering pipelines (on different platforms as well). We cannot drop the existing option(s) and replace them with a new option.

@kevinrushforth, your pseudo code suggestion is in line with what I have implemented albeit a bit differently using two variables. My implementation needs two lines of change to toggle default from OpenGL to Metal.

@prrace
prrace approved these changes Jan 18, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 18, 2021

Mailing list message from Philip Race on lanai-dev:

Alexey,

On 1/11/21 2:11 AM, Alexey Ushakov wrote:

So, we can get rid of (CGraphicsDevice.usingMetalPipeline()) ? ? : ? checks

Best Regards,
Alexey

I think this exactly what I raised on Jan 8th :-

On 1/8/21 11:44 AM, Phil Race wrote:

52: Object context)

53: {
54: return CGraphicsDevice.useMetalPipeline() ? new MTLVolatileSurfaceManager(vImg, context) :
similar to my other naming comments this is more "using" than "use" - or enabled again.
I'd like consistent naming where it makes sense.
But also this looks like another case where I'd prefer to see us have a pattern that retrieves the installed pipeline.
So this entire method body is something like
getInstalledPipeline().createVolatileSurfaceManager(vImg, context);

Our conclusion was to address it in another bug/PR.

-phil.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 18, 2021

Mailing list message from Philip Race on lanai-dev:

We have a situation where this is the pattern which has been adopted on
all other platforms
and it is a very old pattern and lots of tools and people are used to it.
We even have -Dsun.java2d.opengl=... on Windows and Linux so I think it
best to keep this pattern here.
At least here we are thinking about the precedence. There's a bug open
to do the same on Windows and Linux.

-phil.

On 1/15/21 4:05 PM, Alan Snyder wrote:

I must admit I am surprised at how complex the solution is to a simple observation.

Specifically:

1. What is the point of having multiple boolean command line parameters? Instead of separately enabling and disabling pipelines, why not have a single parameter that lists the desired pipelines in priority order. If none of the listed pipelines are available, then the JDK should be free to make its own choice of an available pipeline.

2. I presume this issue is currently a macOS-specific issue, but couldn't a similar issue arise in the future on other platforms? That would argue for using a parameter whose name includes the platform name, so that instructions for multiple platforms (e.g. in some testing script) can be simultaneously specified.

Alan

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks fine to me. You can clean up some redundant logic in a follow-on.

}
} else if (metalState == PropertyState.ENABLED) {
if (oglState == PropertyState.ENABLED) {
oglEnabled = true;

This comment has been minimized.

@kevinrushforth

kevinrushforth Jan 19, 2021
Member

Right. By the time we get here, the two flags cannot possibly be the same value, so you could simplify this by removing this check. That could be done as a follow-up.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 19, 2021

The above comment was made on an older version. I'll redo it on the latest (and also approve the latest).

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good. My earlier comment is still partially valid, so I repeated it in the context of the latest review. It can be done in a follow-up.

@aghaisas
Copy link
Collaborator Author

@aghaisas aghaisas commented Jan 19, 2021

/integrate

@openjdk openjdk bot closed this Jan 19, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jan 19, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 19, 2021

@aghaisas Since your change was applied there have been 46 commits pushed to the master branch:

  • ffff4e1: 8259853: Lanai: nonAA Gradient painting is not precise for VI
  • 61b650c: Automatic merge of jdk:master into master
  • 533a2d3: 8258961: move some fields of SafePointNode from public to protected
  • 6b4732f: 8259679: GitHub actions should use MSVC 14.28
  • 061ffc4: 8249245: assert(((((JfrTraceIdBits::load(klass)) & ((JfrTraceIdEpoch::this_epoch_method_and_class_bits()))) != 0))) failed: invariant
  • e60c992: 8259849: Shenandoah: Rename store-val to IU-barrier
  • db9c114: 7146776: deadlock between URLStreamHandler.getHostAddress and file.Handler.openconnection
  • 61292be: 8259681: Remove the Marlin rendering engine (single-precision)
  • ff275b3: 8259403: Zero: crash with NULL MethodHandle receiver
  • e93f08e: 8074101: Add verification that all tasks are actually claimed during roots processing
  • ... and 36 more: https://git.openjdk.java.net/lanai/compare/98822bb3256ec61b5eb3dee1d88604a7b834121f...master

Your commit was automatically rebased without conflicts.

Pushed as commit 729546e.

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
5 participants