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

8263363: Minor cleanup of Lanai code - unused code removal and comments correction #3357

Closed
wants to merge 6 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -78,7 +78,7 @@ public AffineTransform getNormalizingTransform() {

/**
* Creates a new SurfaceData that will be associated with the given
* CGLLayer.
* layer (CGLLayer/MTLLayer).
*/
public abstract SurfaceData createSurfaceData(CFRetainedResource layer);

@@ -145,9 +145,8 @@ public static MTLGraphicsConfig getConfig(CGraphicsDevice device,
MTLRenderQueue rq = MTLRenderQueue.getInstance();
rq.lock();
try {
// getMTLConfigInfo() creates and destroys temporary
// surfaces/contexts, so we should first invalidate the current
// Java-level context and flush the queue...
// getMTLConfigInfo() creates new MTLContext, so we should first
// invalidate the current Java-level context and flush the queue...
Copy link
Member

@mrserb mrserb Apr 7, 2021

Choose a reason for hiding this comment

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

The old discussion was related not only to the comment but to the invalidateCurrentContext, do we need to do it?

Copy link
Contributor Author

@aghaisas aghaisas Apr 8, 2021

Choose a reason for hiding this comment

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

This is the only place where we use MTLContext.invalidateCurrentContext() - which when processed in MTLRenderQueue - clears some native stuff and nulls out both mtlc and dstOps pointers maintained in MTLRenderQueue.m. I think, this will be important once we get rid of SET_SCRATCH_SURFACE under JDK-8263309.

Copy link
Member

@mrserb mrserb Apr 9, 2021

Choose a reason for hiding this comment

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

But why you need to invalidate context here? Why do you need "clears some native stuff and nulls out both mtlc and dstOps pointers maintained in MTLRenderQueue.m"?

In OGL the getCGLConfigInfo() change the state of the OGL state due to "makeCurrentContext", this is why we need to update the lava level state to "invalid", otherwise we will get a mismatch between the state in the native and java state.

Copy link
Contributor Author

@aghaisas aghaisas Apr 27, 2021

Choose a reason for hiding this comment

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

I see that MTLContext.invalidateCurrentContext() is not needed here. Also, as this is the only place it is used, we can remove the method altogether.

MTLContext.invalidateCurrentContext();
cfginfo = getMTLConfigInfo(displayID, mtlShadersLib);
if (cfginfo != 0L) {
@@ -149,20 +149,6 @@ + (void) _getMTLConfigInfo: (NSMutableArray *)argValue {

NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];


Copy link
Member

@mrserb mrserb Apr 7, 2021

Choose a reason for hiding this comment

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

Please also check how this function is called, looks like previously it was called as a selector+an array as a parameter, and then reworked as a performOnMainThreadWaiting+block, but it still use an array as a parameter. I think it is similar to JDK-8238075.

Copy link
Contributor Author

@aghaisas aghaisas Apr 8, 2021

Choose a reason for hiding this comment

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

Excellent point! Thanks for the pointer to the bug.
A lot of code in this file can be cleaned up. I will update the PR soon.

NSRect contentRect = NSMakeRect(0, 0, 64, 64);
NSWindow *window =
[[NSWindow alloc]
initWithContentRect: contentRect
styleMask: NSBorderlessWindowMask
backing: NSBackingStoreBuffered
defer: false];
if (window == nil) {
J2dRlsTraceLn(J2D_TRACE_ERROR, "MTLGraphicsConfig_getMTLConfigInfo: NSWindow is NULL");
[argValue addObject: [NSNumber numberWithLong: 0L]];
return;
}

MTLContext *mtlc = [[MTLContext alloc] initWithDevice:CGDirectDisplayCopyCurrentMetalDevice(displayID)
shadersLib:mtlShadersLib];
if (mtlc == 0L) {
@@ -171,12 +157,12 @@ + (void) _getMTLConfigInfo: (NSMutableArray *)argValue {
return;
}


// create the MTLGraphicsConfigInfo record for this config
MTLGraphicsConfigInfo *mtlinfo = (MTLGraphicsConfigInfo *)malloc(sizeof(MTLGraphicsConfigInfo));
if (mtlinfo == NULL) {
J2dRlsTraceLn(J2D_TRACE_ERROR, "MTLGraphicsConfig_getMTLConfigInfo: could not allocate memory for mtlinfo");
free(mtlc);
[mtlc release];
mtlc = nil;
[argValue addObject: [NSNumber numberWithLong: 0L]];
return;
}