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

8235772: Remove use of deprecated finalize method from PiscesRenderer and AbstractSurface #66

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2013, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -25,6 +25,8 @@

package com.sun.pisces;

import com.sun.prism.impl.Disposer;

public abstract class AbstractSurface implements Surface {

private long nativePtr = 0L;
@@ -46,6 +48,10 @@
this.height = height;
}

protected void addDisposerRecord() {
Disposer.addRecord(this, new AbstractSurfaceDisposerRecord(nativePtr));
}

public final void getRGB(int[] argb, int offset, int scanLength, int x, int y, int width, int height) {
this.rgbCheck(argb.length, offset, scanLength, x, y, width, height);
this.getRGBImpl(argb, offset, scanLength, x, y, width, height);
@@ -97,8 +103,22 @@ private void rgbCheck(int arr_length, int offset, int scanLength, int x, int y,
}
}

protected void finalize() {
this.nativeFinalize();
private static native void disposeNative(long nativeHandle);

private static class AbstractSurfaceDisposerRecord implements Disposer.Record {
private long nativeHandle;

AbstractSurfaceDisposerRecord(long nh) {
nativeHandle = nh;
}

@Override
public void dispose() {
if (nativeHandle != 0L) {
disposeNative(nativeHandle);
nativeHandle = 0L;
}
}
}

public final int getWidth() {
@@ -108,6 +128,4 @@ public final int getWidth() {
public final int getHeight() {
return height;
}

private native void nativeFinalize();
}
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2016, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -43,6 +43,12 @@ public JavaSurface(int[] dataInt, int dataType, int width, int height) {
this.dataBuffer = IntBuffer.wrap(this.dataInt);

initialize(dataType, width, height);
// The native method initialize() creates the native object of
// struct JavaSurface and saves it's reference in the super class
// member AbstractSurface.nativePtr. This reference is needed for
// creating disposer record hence the below call to addDisposerRecord()
// is needed here and cannot be made in super class constructor.
addDisposerRecord();
This conversation was marked as resolved by kevinrushforth

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Feb 5, 2020

Member

Should this be called from the superclass instead? It works as-is, but if there were ever another subclass added, it would have to be replicated there as well.

This comment has been minimized.

Copy link
@arapte

arapte Feb 7, 2020

Author

AbstractSurface.nativePtr is needed to create the AbstractSurfaceDisposerRecord.
It is created and set by the native method initialize(dataType, width, height); invoked at line 45 in the JavaSurface constructor.
It is the reason addDisposerRecord() is needed here.

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Feb 7, 2020

Member

Oh, I see. The native initialize method in the subclass is writing into a private field in the superclass. Can you add a comment to this effect, since it isn't obvious without reading the native code?

This comment has been minimized.

Copy link
@arapte

arapte Feb 10, 2020

Author

The comment would be really helpful for readers.

}

public IntBuffer getDataIntBuffer() {
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2018, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -25,6 +25,8 @@

package com.sun.pisces;

import com.sun.prism.impl.Disposer;

/**
* PiscesRenderer class is basic public API accessing Pisces library capabilities.
*
@@ -86,6 +88,7 @@
public PiscesRenderer(AbstractSurface surface) {
this.surface = surface;
initialize();
Disposer.addRecord(this, new PiscesRendererDisposerRecord(nativePtr));
}

private native void initialize();
@@ -436,12 +439,21 @@ private void inputImageCheck(int width, int height, int offset, int stride, int
}
}

protected void finalize() {
this.nativeFinalize();
}
private static native void disposeNative(long nativeHandle);

/**
* Native finalizer. Releases native memory used by PiscesRenderer at lifetime.
*/
private native void nativeFinalize();
private static class PiscesRendererDisposerRecord implements Disposer.Record {
private long nativeHandle;

PiscesRendererDisposerRecord(long nh) {
nativeHandle = nh;
}

@Override
public void dispose() {
if (nativeHandle != 0L) {
disposeNative(nativeHandle);
nativeHandle = 0L;
}
}
}
}
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2015, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -37,7 +37,6 @@ static jfieldID fieldIds[SURFACE_LAST + 1];
static jboolean fieldIdsInitialized = JNI_FALSE;

static jboolean initializeSurfaceFieldIds(JNIEnv* env, jobject objectHandle);
static void disposeNativeImpl(JNIEnv* env, jobject objectHandle);

AbstractSurface*
surface_get(JNIEnv* env, jobject surfaceHandle) {
@@ -52,9 +51,14 @@ surface_initialize(JNIEnv* env, jobject surfaceHandle) {
}

JNIEXPORT void JNICALL
Java_com_sun_pisces_AbstractSurface_nativeFinalize(JNIEnv* env,
jobject objectHandle) {
disposeNativeImpl(env, objectHandle);
Java_com_sun_pisces_AbstractSurface_disposeNative(JNIEnv *env, jclass cls, jlong nativePtr)
{
AbstractSurface* surface = (AbstractSurface*) JLongToPointer(nativePtr);

if (surface != NULL) {
surface->cleanup(surface);
surface_dispose(&surface->super);
}
}
This conversation was marked as resolved by kevinrushforth

This comment has been minimized.

Copy link
@johanvos

johanvos Dec 17, 2019

Collaborator

there are no checks on memory errors as used to be in the finalize implementation. Are those not needed anymore?

This comment has been minimized.

Copy link
@arapte

arapte Dec 18, 2019

Author

Hi Johan,
The readAndClearMemErrorFlag() method checks if the variable mem_Error_Flag is JNI_TRUE.
A call should be made to setMemErrorFlag() to set mem_Error_Flag to JNI_TRUE.

The methods used to dispose Surface and renderer only free() the allocated memory and
do not result in making a call to setMemErrorFlag().
So we do not need to check for readAndClearMemErrorFlag()
And I think it was not required before this change too.

This comment has been minimized.

Copy link
@johanvos

johanvos Dec 21, 2019

Collaborator

The dispose methods indeed won't set the mem_Error_Flag but this flag might have been set by other methods. Isn't there a probability that the flag has been set, and that readAndClearMemErrorFlag() is not yet called since the flag has been set?

This comment has been minimized.

Copy link
@arapte

arapte Dec 24, 2019

Author

Hi Johan, I did miss to verify this angle.
But have checked the code now and can confirm that, in a given function for all possible calls to setMemErrorFlag() there exists a call to readAndClearMemErrorFlag() in the same function. So it looks safe to remove the memory checks from dispose()

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Jan 3, 2020

Member

I also looked at the code and don't see any mismatches (including those called from the ACQUIRE_SURFACE macro). I suppose you could restore the call, but since we are in a disposer thread throwing an exception doesn't seem like the right thing to do. You could log the error, but if we are sure there can be no pending errors, it might not be worth the effort.

This comment has been minimized.

Copy link
@arapte

arapte Jan 9, 2020

Author

Yes Kevin, After looking at the code it can be concluded that there are no mismatches. So It seems good to remove those checks. But just in case of safety can keep those as logs too.

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Jan 10, 2020

Member

I don't have a strong preference. Either removing the checks or leaving them in (with a log message rather than an exception) is fine with me.


JNIEXPORT void JNICALL
@@ -184,29 +188,3 @@ initializeSurfaceFieldIds(JNIEnv* env, jobject objectHandle) {

return retVal;
}

static void
disposeNativeImpl(JNIEnv* env, jobject objectHandle) {
This conversation was marked as resolved by arapte

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Feb 5, 2020

Member

Since you removed disposeNativeImpl, you can also remove the static declaration on line 40.

This comment has been minimized.

Copy link
@arapte

arapte Feb 7, 2020

Author

I shall do this in next commit.

AbstractSurface* surface;

if (!fieldIdsInitialized) {
return;
}

surface = (AbstractSurface*)JLongToPointer(
(*env)->GetLongField(env, objectHandle,
fieldIds[SURFACE_NATIVE_PTR]));

if (surface != NULL) {
surface->cleanup(surface);
surface_dispose(&surface->super);
(*env)->SetLongField(env, objectHandle, fieldIds[SURFACE_NATIVE_PTR],
(jlong)0);

if (JNI_TRUE == readAndClearMemErrorFlag()) {
JNI_ThrowNew(env, "java/lang/OutOfMemoryError",
"Allocation of internal renderer buffer failed.");
}
}
}

@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -49,7 +49,6 @@ static jboolean fieldIdsInitialized = JNI_FALSE;
static jboolean initializeRendererFieldIds(JNIEnv *env, jobject objectHandle);

static int toPiscesCoords(unsigned int ff);
static void renderer_finalize(JNIEnv *env, jobject objectHandle);
static void fillAlphaMask(Renderer* rdr, jint minX, jint minY, jint maxX, jint maxY,
JNIEnv *env, jobject this, jint maskType, jbyteArray jmask, jint x, jint y,
jint maskWidth, jint maskHeight, jint offset, jint stride);
@@ -82,14 +81,11 @@ Java_com_sun_pisces_PiscesRenderer_initialize(JNIEnv* env, jobject objectHandle)
}

JNIEXPORT void JNICALL
Java_com_sun_pisces_PiscesRenderer_nativeFinalize(JNIEnv* env,
jobject objectHandle)
Java_com_sun_pisces_PiscesRenderer_disposeNative(JNIEnv *env, jclass cls, jlong nativePtr)
{
renderer_finalize(env, objectHandle);

if (JNI_TRUE == readAndClearMemErrorFlag()) {
JNI_ThrowNew(env, "java/lang/OutOfMemoryError",
"Allocation of internal renderer buffer failed.");
Renderer* rdr = (Renderer*) JLongToPointer(nativePtr);
if (rdr != NULL) {
renderer_dispose(rdr);
}
}

@@ -293,24 +289,6 @@ renderer_get(JNIEnv* env, jobject objectHandle) {
fieldIds[RENDERER_NATIVE_PTR]));
}

static void
renderer_finalize(JNIEnv *env, jobject objectHandle) {
Renderer* rdr;

if (!fieldIdsInitialized) {
return;
}

rdr = (Renderer*)JLongToPointer((*env)->GetLongField(env, objectHandle,
fieldIds[RENDERER_NATIVE_PTR]));

if (rdr != (Renderer*)0) {
renderer_dispose(rdr);
(*env)->SetLongField(env, objectHandle, fieldIds[RENDERER_NATIVE_PTR],
(jlong)0);
}
}

static jboolean
initializeObjectFieldIds(JNIEnv *env,
jobject objectHandle,
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.