On Wed, 16 Jul 2025 19:02:32 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
> first batch of comments (java), mostly questions and minor suggestions. will
> review the native code next.
Thanks for the review Andy...
I addressed most of the comments, but a few which are existing code for es2
pipeline and metal code that is kept similar to es2,d3d pipeline for
similarity, even though the code may not be no-op
A few comments needs to be addressed, I shall update soon.
> modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
> line 334:
>
>> 332:
>> 333: try {
>> 334: FileWriter fragmentShaderHeaderFile = new
>> FileWriter(headerFilesDir + FRAGMENT_SHADER_HEADER_FILE_NAME);
>
> This writer is not buffered, meaning it will be too slow.
> I'd say either to wrap it into `BufferedWriter`, or better yet - use
> `StringBuilder`s everywhere and write the single generated string in one
> operation.
After a change for addressing a review comment, now it mostly uses
StringBuilders except a few String objects.
MSLBackend overall consumes just a few seconds when clean/new build is done.
> modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
> line 336:
>
>> 334: FileWriter fragmentShaderHeaderFile = new
>> FileWriter(headerFilesDir + FRAGMENT_SHADER_HEADER_FILE_NAME);
>> 335:
>> fragmentShaderHeaderFile.write(fragmentShaderHeader.toString());
>> 336: fragmentShaderHeaderFile.write("#endif\n");
>
> this line is weird - why is it here and not after L331?
Yes, it should have been after L331, It is now moved.
> modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
> line 337:
>
>> 335:
>> fragmentShaderHeaderFile.write(fragmentShaderHeader.toString());
>> 336: fragmentShaderHeaderFile.write("#endif\n");
>> 337: fragmentShaderHeaderFile.close();
>
> an exception on lines 335-336 will cause the writer not being closed.
> can we use try-with-resources?
Changed to use try-with-resources.
> modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
> line 346:
>
>> 344:
>> 345: try {
>> 346: FileWriter objCHeaderFile = new FileWriter(headerFilesDir +
>> objCHeaderFileName);
>
> same issue here, need to close the writer properly.
Changed to use try-with-resources.
> modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
> line 423:
>
>> 421:
>> 422: objCHeaderFile.write(" return nil;\n");
>> 423: objCHeaderFile.write("};\n\n");
>
> Not sure if it's worth doing, but perhaps this code can use a single
> StringBuilder to build the code, then use `Files.writeString()` to write it
> (this might require a bit of refactoring).
Changed this particular block to use StringBuilder.
> modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
> line 444:
>
>> 442: }
>> 443:
>> 444: uniformsForShaderFile = uniformsForShaderFile.replace("
>> float2", " vector_float2");
>
> feels like this code was written in 1997. Use a StringBuilder perhaps?
StringBuilder does not offer a method to replace all occurrences of a
particular string. We will have to use combination of indexOf() and replace()
to replace each occurrence individually. It seems like an overkill, given the
MSLBackend is run at build time to generate shaders that consumes a few seconds
when clean/new build is done.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line
> 159:
>
>> 157: @Override
>> 158: protected State updateRenderTarget(RenderTarget target, NGCamera
>> camera, boolean depthTest) {
>> 159: MTLLog.Debug("MTLContext.updateRenderTarget() :target = " +
>> target + ", camera = " + camera + ", depthTest = " + depthTest);
>
> general question: is this temporary? can we use maybe a better logging
> mechanism, one that does not concatenate strings when disabled? works case,
> surround this by `if(debug)` ?
Yes, this is temporary, Kept these for easy analysis if any issues are
observed. Eventually in follow ons we shall remove it.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line
> 172:
>
>> 170:
>> 171: // Validate the camera before getting its computed data
>> 172: if (camera instanceof NGDefaultCamera) {
>
> suggestion: `if (camera instanceof NGDefaultCamera nc)` to avoid explicit
> cast in L173
Changed as suggested.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line
> 180:
>
>> 178: double vw = camera.getViewWidth();
>> 179: double vh = camera.getViewHeight();
>> 180: if (targetWidth != vw || targetHeight != vh) {
>
> would it make sense to detect the case when target(w/h) is very close to
> (w/h) and skip scaling? or is this scenario unlikely?
This is similar to other pipelines, and a different behavior here may cause
visual differences. It would be safe to follow existing impl.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line
> 339:
>
>> 337:
>> 338: public long getMetalCommandQueue()
>> 339: {
>
> I like this style, but the rest of the file uses K&R braces...
Corrected the formatting.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line
> 350:
>
>> 348: // if (checkDisposed()) return;
>> 349: // nSetDeviceParametersFor2D(pContext);
>> 350: }
>
> remove the commented out code LL348-349, leave the comment?
Removed the two lines keeping comment.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line
> 357:
>
>> 355: // result of this call, hence the method is no-op.
>> 356: // But overriding the method here for any future reference.
>> 357: // if (checkDisposed()) return;
>
> same here?
Removed the two lines keeping comment.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line
> 489:
>
>> 487: }
>> 488:
>> 489: private void printRawMatrix(String mesg) {
>
> unused method?
The calls to this method are commented out. It would be handy to have the
matrix printing mechanism handy in case of debugging, similarly it is present
in other pipelines.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLMesh.java line 60:
>
>> 58: @Override
>> 59: public void dispose() {
>> 60: disposerRecord.dispose();
>
> two questions here:
> 1. why is `disposerRecord.dispose()`; here (and in `D3DMesh`, `ES2Mesh`) and
> not in the `BaseGraphicResource` ?
> 2. any danger in the `dispose()` being called twice? (could it happen at
> all?)
1. BaseGraphicResource is inherited by other resource classes like Shader and
Texture too. It seems good idea to implement dispose() all in child classes as
needed.
2. Calling twice is never observed in renderperf tests with large number of
objects, and seems not possible, but deep analysis for these things can be done
only with stress based scenarios.
As these are similar to other pipelines, It can be common analysis for future
keeping it out of scope for this PR.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLMesh.java line 93:
>
>> 91: }
>> 92:
>> 93: void traceDispose() {}
>
> what's the purpose of this method?
It seems it's purpose was to track allocation and deallocation of resources.
But the method is empty in most classes except ES2TextureData.java. Instead of
removing this needs better handling by improving PrismTrace class and giving a
definition to these empty traceDispose() methods.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLPhongMaterial.java
> line 104:
>
>> 102: String logname = PhongMaterial.class.getName();
>> 103: PlatformLogger.getLogger(logname).warning(
>> 104: "Warning: Low on texture resources. Cannot
>> create texture.");
>
> I suppose this message is the same as elsewhere. Is it clear enough for the
> user? Does the user have a clear idea of how to correct the issue?
Yes, it is same for other pipelines too. It looks more developer friendly, but
it does offer basic insight.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLPipeline.java line
> 90:
>
>> 88:
>> 89: // This enables sharing of MTLCommandQueue between PRISM and
>> GLASS
>> 90: HashMap devDetails = (HashMap)
>> MTLPipeline.getInstance().getDeviceDetails();
>
> cast is unnecessary. suggestion:
> `Map devDetails = MTLPipeline.getInstance().getDeviceDetails();`
>
> I guess we've inherited the API based on the raw object here?
Removed the cast by using above suggestion.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLRTTextureData.java
> line 35:
>
>> 33: @Override
>> 34: public void dispose() {
>> 35: if (pTexture != 0L) {
>
> this code is weird: why doesn't call super.dispose()?
Changed to call super.dispose()
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java
> line 75:
>
>> 73: i *= 2;
>> 74: }
>> 75: return i;
>
> interesting... would `1 << val` be simpler (barring overflow)?
Changed accordingly and included another check to verify if the `val` is
already a power of two.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java
> line 128:
>
>> 126: return (Shader) m.invoke(null, new Object[]{this,
>> shaderName, nameStream});
>> 127: } catch (Throwable e) {
>> 128: e.printStackTrace();
>
> do we want to use a logger or the `stderr` is the best decision in this case?
There is no use of Logger in the prism rendering pipeline implementation.
Perhaps we should change all stderr to Logger but outside of this PR.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java
> line 183:
>
>> 181:
>> 182: long pResource = nCreateTexture(context.getContextHandle() ,
>> 183: (int) formatHint.ordinal(), (int) usageHint.ordinal(),
>
> unnecessary casts to (int) x2
Removed the cast
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java
> line 274:
>
>> 272: Texture tex = createTexture(texFormat, Usage.DEFAULT,
>> WrapMode.CLAMP_TO_EDGE, texWidth, texHeight);
>> 273:
>> 274: frame.releaseFrame();
>
> could all these `frame.releaseFrame()` be moved to a `finally` block?
Added a finally block.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java
> line 305:
>
>> 303: @Override
>> 304: public int getRTTWidth(int w, Texture.WrapMode wrapMode) {
>> 305: // Below debugging logic replicates D3DResoureFactory
>
> is this code still needed? also L314
There is more detailed description in D3DResouceFactory. This comment here
would serve as good redirection if one is looking into it. Let's keep it.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java
> line 396:
>
>> 394: public void dispose() {
>> 395: super.dispose();
>> 396: context.dispose();
>
> Q: should we call super.dispose() _after_ context.dispose()?
Yes, Changed the order. Any resources being used by MTLCommandQueue are made
resident, so the resources would stay alive even if MTLResourceFactory is
disposed. but the earlier order was incorrect.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLSwapChain.java
> line 128:
>
>> 126:
>> 127: if (pState.getNativeFrameBuffer() == 0) {
>> 128: System.err.println("Native backbuffer texture from Glass is
>> nil.");
>
> is `stderr` ok here?
There is no use of Logger in the prism rendering pipeline implementation.
Perhaps we should change all stderr to Logger but outside of this PR.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLSwapChain.java
> line 143:
>
>> 141: stableBackbuffer = null;
>> 142: } /*else {
>> 143: // RT-27554
>
> is this still relevant?
It is not relevant to metal, removed.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLTexture.java line
> 97:
>
>> 95: int srcscan, boolean skipFlush) {
>> 96:
>> 97: if (format.getDataType() == PixelFormat.DataType.INT) {
>
> would a `switch` be better here?
Agreed, changed to switch.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLTexture.java line
> 122:
>
>> 120: byte[] arr = buf.hasArray() ? buf.array() : null;
>> 121:
>> 122: if (format == PixelFormat.BYTE_BGRA_PRE || format ==
>> PixelFormat.BYTE_ALPHA) {
>
> also here, `switch` ?
Agreed, changed to switch.
> modules/javafx.graphics/src/main/native-glass/mac/GlassCGLOffscreen.h line 45:
>
>> 43: GLuint _textureWidth;
>> 44: GLuint _textureHeight;
>> 45: NSView* glassView;
>
> minor: we probably should not try to align, but simply use a single space
> char, as in java code.
I prefer a few spaces if they make code look easy on eyes, here the spaces are
a little more, but this is a pre-existing code for es2 pipeline, In this PR it
is only moved to a new file. GlassOffscreen.m to Glass**CGL**Offscreen.m
> modules/javafx.graphics/src/main/native-glass/mac/GlassCGLOffscreen.m line 48:
>
>> 46: {
>> 47: // TODO: implement PBuffer if needed
>> 48: // self->_fbo = [[GlassPBuffer alloc] init];
>
> commented out - is it going to crash of the value is `nil`?
This is a pre-existing code for es2 pipeline, In this PR it is only moved to a
new file. GlassOffscreen.m to Glass**CGL**Offscreen.m
> modules/javafx.graphics/src/main/native-glass/mac/GlassCGLOffscreen.m line
> 143:
>
>> 141:
>> withObject:nil
>> 142:
>> waitUntilDone:NO
>> 143:
>> modes:allModes];
>
> minor: this indentation is hard to look at
Changed the indentation.
> modules/javafx.graphics/src/main/native-glass/mac/GlassMTLOffscreen.m line 68:
>
>> 66: [(NSObject*)self->_fbo release];
>> 67: self->_fbo = nil;
>> 68: [super dealloc];
>
> here and elsewhere: should we protect against repeated calls to dealloc?
> like `if(self->_fbo != nil)`
Updated the code with null check.
> modules/javafx.graphics/src/main/native-prism-mtl/MetalPipelineManager.m line
> 32:
>
>> 30: // ---------------------------- Debug helper for Developers
>> -------------------------
>> 31: //
>> 32: // This implementation is to utilize "Metal Debuger" present in Xcode.
>
> is the debugger available in production code?
It is available to developers only. It requires two things
Un-comment line 51: //#define JFX_MTL_DEBUG_CAPTURE
and enable metal debug trace capturing, on terminal, export
MTL_CAPTURE_ENABLED=1
> modules/javafx.graphics/src/main/native-prism-mtl/MetalRingBuffer.m line 34:
>
>> 32: static unsigned int currentBufferIndex;
>> 33:
>> 34: - (MetalRingBuffer*) init:(MetalContext*)ctx
>
> ring buffers are a frequent source of off-by-one bugs.
> is it possible to have a unit test for it?
It would require test specific helper methods to add the tests. I think it
should be out of this PR as a followup.
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1824#issuecomment-3090250301
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216602426
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216602524
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216602217
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216602288
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216602618
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216602696
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216602902
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216603131
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216603253
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216603339
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216603454
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216603564
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216603729
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216604023
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216604132
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216604269
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216604415
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216604868
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216605130
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216605310
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216605420
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216605635
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216605806
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216605941
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216606136
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216606220
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216606435
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216606559
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216606903
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216607159
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216607254
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216607473
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216607630
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2216611680
