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

Conversation

@arapte
Copy link

arapte commented Dec 11, 2019

The finalize() method is deprecated in JDK9. See Java 9 deprecated features.
And so the PiscesRenderer.finalize() and AbstractSurface.finalize() method should be removed.

The change is,

  1. Remove finalize method from PiscesRenderer and AbstractSurface classes.
  2. Use Disposer class for cleanup instead of finalize(). For SW pipeline the cleanup is initiated in here in UploadingPainer.java.
  3. Added new JNI methods to JPiscesRenderer.c and JAbstractSurface.c and removed the ones which were used previously with finalize() method.

Verification:
Verified that the newly added dispose() methods get invoked as required and no OOM occurs.

  1. Create a sample program which adds and removes non 3D shapes and/or controls to stage in a loop.
  2. Run the program using -Dprism.order=sw, -Dprism.verbose=true. (optional -Xmx50m)
    Expected Behavior:
    a. Maximum memory consumption after this change should reduce or remain same as that of before this change.
    b. No OOM should occur with this change or without this change.

Progress

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

Issue

  • JDK-8235772: Remove use of deprecated finalize method from PiscesRenderer and AbstractSurface

Reviewers

  • Johan Vos (jvos - Reviewer)
  • Kevin Rushforth (kcr - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/66/head:pull/66
$ git checkout pull/66

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 11, 2019

👋 Welcome back arapte! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

@arapte arapte changed the title [WIP]8235772: Remove use of deprecated finalize method from PiscesRenderer… 8235772: Remove use of deprecated finalize method from PiscesRenderer and AbstractSurface Dec 11, 2019
@arapte arapte marked this pull request as ready for review Dec 11, 2019
@openjdk openjdk bot added the rfr label Dec 11, 2019
@mlbridge
Copy link

mlbridge bot commented Dec 11, 2019

Webrevs

@kevinrushforth
Copy link
Member

kevinrushforth commented Dec 11, 2019

This will need two reviewers.

@nlisker
Copy link
Contributor

nlisker commented Dec 11, 2019

Isn't Pisces going to be completely removed anyway?

@kevinrushforth
Copy link
Member

kevinrushforth commented Dec 11, 2019

The Pisces rasterizer will be removed, but as it turns out, the two classes in question are used by the prism-sw pipeline as part of the Pisces-based software renderer.

See the comment I added to JDK-8196079.

@kevinrushforth
Copy link
Member

kevinrushforth commented Jan 24, 2020

/reviewers 2

@openjdk
Copy link

openjdk bot commented Jan 24, 2020

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

Copy link
Collaborator

johanvos left a comment

I did some more tests, and it looks good.
There might be a merge conflict, as the date for JPiscesRenderer was modified.

… and AbstractSurface
@arapte arapte force-pushed the arapte:PiscesRenderer_remove_finalize branch from 31df0eb to 0591e41 Jan 29, 2020
Copy link
Member

kevinrushforth left a comment

The code changes look good to me, with a couple minor points. I tested it and it does what I would expect.

Ambarish Rapte
@kevinrushforth
Copy link
Member

kevinrushforth commented Feb 10, 2020

@johanvos the Skara team enabled the invalidation of stale reviews. Can you re-review this when you get a chance?

Btw, it isn't clear from looking at this that your approval no longer counts; they are working to improve the presentation of this so it will be more clear.

@openjdk
Copy link

openjdk bot commented Feb 19, 2020

@arapte This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

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

Reviewed-by: jvos, kcr
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 27 commits pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 31505620ad5fc49580ae7e7241a1edcd6d1f2e30.

➡️ To integrate this PR with the above commit message, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Feb 19, 2020
@arapte
Copy link
Author

arapte commented Feb 24, 2020

/integrate

@openjdk openjdk bot closed this Feb 24, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Feb 24, 2020
@openjdk
Copy link

openjdk bot commented Feb 24, 2020

@arapte The following commits have been pushed to master since your change was applied:

  • 4eaff0d: 8239109: Update SQLite to version 3.31.1
  • d8e7f85: 8239454: LLIntData : invalid opcode returned for 16 and 32 bit wide instructions
  • 48ddd80: Merge
  • 35a01ca: 8228867: Fix mistakes in FX API docs
  • b5e65ec: 8238434: Ensemble: Update version of Lucene to 7.7.2
  • e986459: 8227619: Potential memory leak in javafx.scene.control.ListView
  • a74137a: 8236839: System menubar removed when other menubars are created or modified
  • 21d3b7e: 8237453: [TabPane] Incorrect arrow key traversal through tabs after reordering
  • e224e54: 8238526: Cherry pick GTK WebKit 2.26.3 changes
  • 6968e38: 8237469: Inherited styles don't update when node is moved
  • 3c55a7e: Merge
  • a4b9f24: 8237503: Update copyright header for files modified in 2020
  • 1749e85: 8237975: Non-embedded Animations do not play backwards after being paused
  • 2ab40c1: 8231513: JavaFX cause Keystroke Receiving prompt on MacOS 10.15 (Catalina)
  • aa91ebb: 8237944: webview native cl "-m32" unknown option for windows 32-bit build
  • 56267e1: 8237770: Error creating fragment phong shader on iOS
  • 95bf2c0: 8237782: Only read advances up to the minimum of the numHorMetrics or the available font data.
  • d05e8fc: 8237833: Check glyph size before adding to glyph texture cache
  • 1213ea7: Merge
  • 1823f6e: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
  • ca37c1f: 8238249: GetPrimitiveArrayCritical passed with hardcoded FALSE value
  • b96bc52: 8237003: Remove hardcoded WebAnimationsCSSIntegrationEnabled flag in DumpRenderTree
  • d303a21: Merge
  • 5a0e71b: 8237372: NullPointerException in TabPaneSkin.stopDrag
  • 79fc0d0: 8232824: Removing TabPane with strong referenced content causes memory leak from weak one
  • aa6f3a4: 8236912: NullPointerException when clicking in WebView with Button 4 or Button 5

Your commit was automatically rebased without conflicts.

Pushed as commit 3150562.

@arapte arapte deleted the arapte:PiscesRenderer_remove_finalize branch Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.