JDK-8178804 : Excessive memory consumption in TriangleMesh/MeshView
  • Type: Bug
  • Component: javafx
  • Sub-Component: graphics
  • Affected Version: 8u60,9
  • Priority: P2
  • Status: Resolved
  • Resolution: Fixed
  • OS: os_x
  • CPU: x86
  • Submitted: 2017-04-13
  • Updated: 2020-01-31
  • Resolved: 2017-04-21
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 8 JDK 9
8u152Fixed 9Fixed
Related Reports
Relates :  
Relates :  
Description
FULL PRODUCT VERSION :
JDK 8u152 ea (and other recent versions)

ADDITIONAL OS VERSION INFORMATION :
OS X 10.10.5 MacBook Pro (Retina) NVIDIA GeForce GT 650M 1024 MB

A DESCRIPTION OF THE PROBLEM :
This is a test case to show the excessive memory consumption in TriangleMesh/MeshView
as I was asked for in this mail on the openjfx mailing list.

http://mail.openjdk.java.net/pipermail/openjfx-dev/2017-April/020417.html

In its default setup the test case shows a triangle mesh which displays
10,000 rectangles (built via two triangles each). For simplicity reasons I am
only tracking the float arrays here because they dominate the final memory consumption.

These are the results I get via VisualVM:

float[] memory (after enforced garbage collection)
   480,068 bytes nominal memory (definition of mesh)
 1,494,920 bytes actual memory before BaseMesh.buildGeometry
27,416,832 bytes actual memory after  BaseMesh.buildGeometry

Ratio: 1:57

The test case was developed to examine the usefulness of a triangle mesh
to do accelerated 2D graphics so it may be a bit special (identical normals
for all vertices and a special usage of the artificial texture). But
even if the ratio is just 1:30 or so in a more typical case this is still
far too much.

The origin of the problem seems to be that the BaseMesh class keeps
all the intermediate memory in order to be able to do a fast update
of the geometry later. But in many user scenarios this will never
happen because you just load a 3D model once and then just move it around.
In practice it is often much more important to conserve resources.

It might be worthwhile to consider a flag where the user can indicate
his specific priorities.


STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
Run the attached test program and meassure the memory consumption via some tool like VisualVM.

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
A much lower memory consumption.
ACTUAL -
A 57X increase of the float[] memory compared to the nominal memory needed to define the mesh.

REPRODUCIBILITY :
This bug can be reproduced always.

---------- BEGIN SOURCE ----------
https://www.dropbox.com/s/3q2pegejefimv05/trianglemesh.zip?dl=0
---------- END SOURCE ----------


Comments
+1 Approved to push to 8u-dev for 8u152.
18-05-2017

Webrev for backport to 8udev: http://cr.openjdk.java.net/~ckyang/JDK-8178804/Backport/webrev.00/
25-04-2017

Changeset: 6efa37ea0d1c Author: ckyang Date: 2017-04-21 16:47 -0700 URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/6efa37ea0d1c
21-04-2017

After pushing this to 9-dev, go ahead and post a webrev for JDK 8u and I will evaluate it. We'll want to let it bake in JDK 9 for a week or so anyway.
21-04-2017

Fix request approved This is can now be pushed to FX 9-dev for JDK 9.
21-04-2017

Looks fine now, thanks.
21-04-2017

Thanks! Here is webrev.03: http://cr.openjdk.java.net/~ckyang/JDK-8178804/webrev.03/ and the only meaningful diff between webrev.03 and webrev.02: < --- /dev/null 2017-04-21 08:45:09.000000000 -0700 < +++ new/tests/system/src/test/java/test/com/sun/prism/impl/PNTMeshVertexBufferLengthTest.java 2017-04-21 08:45:09.000000000 -0700 --- > --- /dev/null 2017-04-20 05:10:33.942416009 -0700 > +++ new/tests/system/src/test/java/test/com/sun/prism/impl/PNTMeshVertexBufferLenghtTest.java 2017-04-20 14:26:34.527819425 -0700 261c261 < +public class PNTMeshVertexBufferLengthTest { --- > +public class PNTMeshVertexBufferLenghtTest { 429c429 < + PNTMeshVertexBufferLengthTest.myApp = this; --- > + PNTMeshVertexBufferLenghtTest.myApp = this; 434c434 < + primaryStage.setTitle("PNTMeshVertexBufferLengthTest"); --- > + primaryStage.setTitle("PNTMeshVertexBufferLenghtTest");
21-04-2017

My sanity testing looks good. +1 pending the rename of the test to fix the typo. Since this is an RDP 2 request, I would like to see an updated webrev before approving it to go into JDK 9.
21-04-2017

The .02 webrev looks good to me, except for one typo in the name of the test class: tests/system/src/test/java/test/com/sun/prism/impl/PNTMeshVertexBufferLenghtTest.java Lenght should be Length (also appears in the title of the Stage). I will run a full test with the patch applied and then finish the review.
21-04-2017

Fix Request a) Why it's important to fix this bug: This bug has an unacceptable data buffers growth rate (9X) that causes excessive Java side memory consumption in TriangleMesh. b) Explain the nature of fix: The fix is to divide vbCount by VERTEX_SIZE_VB when computing the new vertex count. And to ensure a reasonable growth rate of the data buffers we pick a 12.5% or 6 vertices which ever is greater. c) Estimate its risk: This is low risk fix and the change is limited to a few lines of code within the data growth code segment. d) Describe its test coverage: A total of 5 unit tests were added to cover all cases of growth. The tests cover empty mesh, mesh that only trigger fit growth of 6 vertices, mesh that starts from fit growth and crossover to 12.5% growth rate and mesh that grows at 12.5% growth rate. e) Reviewer: kcr was tagged.
20-04-2017

Here is the revised webrev.02: http://cr.openjdk.java.net/~ckyang/JDK-8178804/webrev.02/ The only change made from 01 is to use arithmetic shift operator and updated last 2 test cases due to change in growth rate. I also filed a RFE, JDK-8179039, for 10 to have proper handling of data overflow in TriangleMesh.
20-04-2017

Thank you for the feedback. Since this is a late bug fix for 9 and a backport candidate for 8u we would want to keep this fix as targeted as possible. I would propose improving this fix by avoiding the floating-point multiplication and integer cast as suggested here. And address the possible overflow issue in a separate RFE for 10 as it will need more evaluation time and testing of the Mesh as a whole. We can include the replacement of System.arraycopy() with Arrays.copyOf() in that work.
20-04-2017

I had a look to the patch (but I am not a reviewer) and I would recommend the fix to implement overflow protection as I did in Path2D.expandCoords(), because array lengths are integer values that can overflow 2^31 (then become negative): - final int newNumVertices = numVertices + Math.max((int) (numVertices * 0.1), 6); (numVertices * 0.1) is a double value so could exceed the integer capacity and the cast will return Integer.MAX_VALUE ! You could use integer maths to avoid the floating-point multiplication + integer cast: just use numVertices + (numVertices >>> 3) ie 1/8th increment. - float[] temp = new float[newNumVertices * VERTEX_SIZE_VB]; or temp = new float[newNumVertices * 3]; Of course, numVertices * 3 or * 6 will then be negative in case of overflow. Please check if the new capacity do not overflow. FYI In Path2D, we estimate the new capacity and adjust it below the Integer.MAX_VALUE if the capacity is enough and fail if not. Finally you could use Arrays.copyOf() to simplify such code: temp = new float[newNumVertices * 3]; System.arraycopy(cachedNormals, 0, temp, 0, cachedNormals.length); cachedNormals = temp; => cachedNormals = Arrays.copyOf(cachedNormals, newSize)
20-04-2017

Thanks for the feedback. I have forgotten that the tests are run in random order and it so happen that the assertion didn't occur during my testing. I have rewritten the tests to use individual mesh per test and verified that this fixes the tests on multiple machines. Here is the revised webrev.01 which includes all your feedback: http://cr.openjdk.java.net/~ckyang/JDK-8178804/webrev.01/
20-04-2017

The fix itself looks correct. The test looks quite good, too, but there are a few issues. * I get a test failure with the 0 divisions test. test.com.sun.prism.impl.PNTMeshVertexBufferLenghtTest > testMeshWithZeroDiv FAILED java.lang.AssertionError: expected:<0> but was:<15000> at org.junit.Assert.fail(Assert.java:91) at org.junit.Assert.failNotEquals(Assert.java:645) at org.junit.Assert.assertEquals(Assert.java:126) at org.junit.Assert.assertEquals(Assert.java:470) at org.junit.Assert.assertEquals(Assert.java:454) at test.com.sun.prism.impl.PNTMeshVertexBufferLenghtTest.testMeshWithZeroDiv(PNTMeshVertexBufferLenghtTest.java:265) I think the cause is that you reuse the static triangle mesh created in the setupOnce() method. It would be better to remove the old Mesh (and MeshView) and construct a new one for each test. I also see three minor issues that need to be addressed. 1. BUG: Missing the copyright header for PNTMeshVertexBufferLenghtTest.java and Vec3f.java 2. BUG: Wrong copyright year for newly added BaseMeshShim.java Also, as long as you are updating the webrev for the previous two, you might also want to update the copyright year for NGTriangleMesh.java and BaseMesh.java (up to you). 3. TYPO: The following comment is wrong: 302 // vertexBuffer started with 6 vertices and grew by 6, 3 times, to a 303 // capacity of 27 vertices (27 * 9 = 243) That should be "started with 9 vertices..." Additionally, here are some optional formatting / style nits. Probably only the first two are worth considering... 4. NIT: extra space after the '[' int faces[] = new int[ faceCount * faceSize]; 5. NIT: You call assertXXXX with a mix of static imports and not. FOr example: assertEquals(0, BaseMeshShim.test_getNumberOfVertices(baseMesh)); Assert.assertTrue(BaseMeshShim.test_isVertexBufferNull(baseMesh)); 6. COMMENT: might it be cleaner to "do the math" in code rather than the comment? Not a big deal, though. final int expectedVBLength = 27 * VERTEX_SIZE; 7. NIT: the javafx.graphics/com.sun.javafx.sg.prism=ALL-UNNAMED was added such that the entries are no longer sorted.
19-04-2017

Filed an enhancement request, JDK-8178974, for 10 to remove cached data based on hint provided by user.
19-04-2017

Please review the proposed fix. The really fix is a 3 lines fix in BaseMesh.java but I also took the chance to rename the local variable to make the code segment more readable. http://cr.openjdk.java.net/~ckyang/JDK-8178804/webrev.00/
19-04-2017

Retargeting to 9. Given that a safe fix can be found, this is a good candidate to consider for getting into 9 via the RDP2 request process. Also, this is a good candidate to consider for a backport to 8u.
18-04-2017

Raising to P2 as this is a serious memory consumption bug with no good application workaround.
18-04-2017

There is a missing divide (by VERTEX_SIZE_VB) when computing the growth of vertexBuffer array as needed. Hence it has the potential of creating a vertexBuffer and some cached data to 9 times as big as designed. However this bug only affect Mesh with VertexFormat.POINT_NORMAL_TEXCOORD.
17-04-2017