-
Notifications
You must be signed in to change notification settings - Fork 3.7k
General performance improvements #12968
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
Changes from all commits
dd82a80
599f11a
e699c1c
7585f64
f67f8dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,28 +35,30 @@ import CesiumMath from "./Math.js"; | |
| * @see Matrix2 | ||
| * @see Matrix4 | ||
| */ | ||
| function Matrix3( | ||
| column0Row0, | ||
| column1Row0, | ||
| column2Row0, | ||
| column0Row1, | ||
| column1Row1, | ||
| column2Row1, | ||
| column0Row2, | ||
| column1Row2, | ||
| column2Row2, | ||
| ) { | ||
| this[0] = column0Row0 ?? 0.0; | ||
| this[1] = column0Row1 ?? 0.0; | ||
| this[2] = column0Row2 ?? 0.0; | ||
| this[3] = column1Row0 ?? 0.0; | ||
| this[4] = column1Row1 ?? 0.0; | ||
| this[5] = column1Row2 ?? 0.0; | ||
| this[6] = column2Row0 ?? 0.0; | ||
| this[7] = column2Row1 ?? 0.0; | ||
| this[8] = column2Row2 ?? 0.0; | ||
| class Matrix3 extends Float64Array { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extended the improvements on |
||
| constructor( | ||
| column0Row0, | ||
| column1Row0, | ||
| column2Row0, | ||
| column0Row1, | ||
| column1Row1, | ||
| column2Row1, | ||
| column0Row2, | ||
| column1Row2, | ||
| column2Row2, | ||
| ) { | ||
| super(9); | ||
| this[0] = column0Row0 ?? 0.0; | ||
| this[1] = column0Row1 ?? 0.0; | ||
| this[2] = column0Row2 ?? 0.0; | ||
| this[3] = column1Row0 ?? 0.0; | ||
| this[4] = column1Row1 ?? 0.0; | ||
| this[5] = column1Row2 ?? 0.0; | ||
| this[6] = column2Row0 ?? 0.0; | ||
| this[7] = column2Row1 ?? 0.0; | ||
| this[8] = column2Row2 ?? 0.0; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * The number of elements used to pack the object into an array. | ||
| * @type {number} | ||
|
|
@@ -1671,19 +1673,15 @@ Matrix3.equalsEpsilon = function (left, right, epsilon) { | |
| * @type {Matrix3} | ||
| * @constant | ||
| */ | ||
| Matrix3.IDENTITY = Object.freeze( | ||
| new Matrix3(1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0), | ||
| ); | ||
| Matrix3.IDENTITY = new Matrix3(1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the limitation that it's not possible to The identity and zero matrices are used frequently across CesiumJS and I'd assume other projects that use CesiumJS. There needs to be a way to guarantee these are always the expected values. Users should not be allowed to do this: console.log(Cesium.Matrix3.IDENTITY);
Cesium.Matrix3.IDENTITY[2] = 27.0;
console.log(Cesium.Matrix3.IDENTITY);Currently this code throws an error but in this branch this works just fine and the identity is now wrong in every place it's used. It's possible this could become a getter style function that always returns a new matrix but that defeats the purpose of having a shared object and could create more memory issues as it creates new objects every time. Are there ways to mix frozen, normal arrays and typed arrays? Is there a different way to change internal structure based on expected mutability? Would that just make it more painful and inconsistent to use? I don't know the answer to these questions but we need to figure it out before pushing forward with this change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no issue mixing old object matrices and new array ones, so keeping these as old frozen object based matrices is a valid solution. However:
If after further consideration you believe it's still important these specifically can be reverted to frozen object matrices There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets continue this conversation in #12973 |
||
|
|
||
| /** | ||
| * An immutable Matrix3 instance initialized to the zero matrix. | ||
| * | ||
| * @type {Matrix3} | ||
| * @constant | ||
| */ | ||
| Matrix3.ZERO = Object.freeze( | ||
| new Matrix3(0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0), | ||
| ); | ||
| Matrix3.ZERO = new Matrix3(0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0); | ||
|
|
||
| /** | ||
| * The index into Matrix3 for column 0, row 0. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,40 +53,43 @@ import RuntimeError from "./RuntimeError.js"; | |
| * @see Matrix3 | ||
| * @see Packable | ||
| */ | ||
| function Matrix4( | ||
| column0Row0, | ||
| column1Row0, | ||
| column2Row0, | ||
| column3Row0, | ||
| column0Row1, | ||
| column1Row1, | ||
| column2Row1, | ||
| column3Row1, | ||
| column0Row2, | ||
| column1Row2, | ||
| column2Row2, | ||
| column3Row2, | ||
| column0Row3, | ||
| column1Row3, | ||
| column2Row3, | ||
| column3Row3, | ||
| ) { | ||
| this[0] = column0Row0 ?? 0.0; | ||
| this[1] = column0Row1 ?? 0.0; | ||
| this[2] = column0Row2 ?? 0.0; | ||
| this[3] = column0Row3 ?? 0.0; | ||
| this[4] = column1Row0 ?? 0.0; | ||
| this[5] = column1Row1 ?? 0.0; | ||
| this[6] = column1Row2 ?? 0.0; | ||
| this[7] = column1Row3 ?? 0.0; | ||
| this[8] = column2Row0 ?? 0.0; | ||
| this[9] = column2Row1 ?? 0.0; | ||
| this[10] = column2Row2 ?? 0.0; | ||
| this[11] = column2Row3 ?? 0.0; | ||
| this[12] = column3Row0 ?? 0.0; | ||
| this[13] = column3Row1 ?? 0.0; | ||
| this[14] = column3Row2 ?? 0.0; | ||
| this[15] = column3Row3 ?? 0.0; | ||
| class Matrix4 extends Float64Array { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The most important improvement here. Matrices are at the heart of 3D graphics, and any performance improvement here radiates throughout the entire codebase. I approached this primarily to speed up By making Matrix4 a class extending
I show the performance improvements of this combined with @javagl 's branch #12658 in #11814. Importantly, changing the backing datastructure to |
||
| constructor( | ||
| column0Row0, | ||
| column1Row0, | ||
| column2Row0, | ||
| column3Row0, | ||
| column0Row1, | ||
| column1Row1, | ||
| column2Row1, | ||
| column3Row1, | ||
| column0Row2, | ||
| column1Row2, | ||
| column2Row2, | ||
| column3Row2, | ||
| column0Row3, | ||
| column1Row3, | ||
| column2Row3, | ||
| column3Row3, | ||
| ) { | ||
| super(16); | ||
| this[0] = column0Row0 ?? 0.0; | ||
| this[1] = column0Row1 ?? 0.0; | ||
| this[2] = column0Row2 ?? 0.0; | ||
| this[3] = column0Row3 ?? 0.0; | ||
| this[4] = column1Row0 ?? 0.0; | ||
| this[5] = column1Row1 ?? 0.0; | ||
| this[6] = column1Row2 ?? 0.0; | ||
| this[7] = column1Row3 ?? 0.0; | ||
| this[8] = column2Row0 ?? 0.0; | ||
| this[9] = column2Row1 ?? 0.0; | ||
| this[10] = column2Row2 ?? 0.0; | ||
| this[11] = column2Row3 ?? 0.0; | ||
| this[12] = column3Row0 ?? 0.0; | ||
| this[13] = column3Row1 ?? 0.0; | ||
| this[14] = column3Row2 ?? 0.0; | ||
| this[15] = column3Row3 ?? 0.0; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -1929,41 +1932,23 @@ Matrix4.multiplyTransformation = function (left, right, result) { | |
| const right13 = right[13]; | ||
| const right14 = right[14]; | ||
|
|
||
| const column0Row0 = left0 * right0 + left4 * right1 + left8 * right2; | ||
| const column0Row1 = left1 * right0 + left5 * right1 + left9 * right2; | ||
| const column0Row2 = left2 * right0 + left6 * right1 + left10 * right2; | ||
|
|
||
| const column1Row0 = left0 * right4 + left4 * right5 + left8 * right6; | ||
| const column1Row1 = left1 * right4 + left5 * right5 + left9 * right6; | ||
| const column1Row2 = left2 * right4 + left6 * right5 + left10 * right6; | ||
|
|
||
| const column2Row0 = left0 * right8 + left4 * right9 + left8 * right10; | ||
| const column2Row1 = left1 * right8 + left5 * right9 + left9 * right10; | ||
| const column2Row2 = left2 * right8 + left6 * right9 + left10 * right10; | ||
|
|
||
| const column3Row0 = | ||
| left0 * right12 + left4 * right13 + left8 * right14 + left12; | ||
| const column3Row1 = | ||
| left1 * right12 + left5 * right13 + left9 * right14 + left13; | ||
| const column3Row2 = | ||
| left2 * right12 + left6 * right13 + left10 * right14 + left14; | ||
|
|
||
| result[0] = column0Row0; | ||
| result[1] = column0Row1; | ||
| result[2] = column0Row2; | ||
| result[0] = left0 * right0 + left4 * right1 + left8 * right2; | ||
| result[1] = left1 * right0 + left5 * right1 + left9 * right2; | ||
| result[2] = left2 * right0 + left6 * right1 + left10 * right2; | ||
| result[3] = 0.0; | ||
| result[4] = column1Row0; | ||
| result[5] = column1Row1; | ||
| result[6] = column1Row2; | ||
| result[4] = left0 * right4 + left4 * right5 + left8 * right6; | ||
| result[5] = left1 * right4 + left5 * right5 + left9 * right6; | ||
| result[6] = left2 * right4 + left6 * right5 + left10 * right6; | ||
| result[7] = 0.0; | ||
| result[8] = column2Row0; | ||
| result[9] = column2Row1; | ||
| result[10] = column2Row2; | ||
| result[8] = left0 * right8 + left4 * right9 + left8 * right10; | ||
| result[9] = left1 * right8 + left5 * right9 + left9 * right10; | ||
| result[10] = left2 * right8 + left6 * right9 + left10 * right10; | ||
| result[11] = 0.0; | ||
| result[12] = column3Row0; | ||
| result[13] = column3Row1; | ||
| result[14] = column3Row2; | ||
| result[12] = left0 * right12 + left4 * right13 + left8 * right14 + left12; | ||
| result[13] = left1 * right12 + left5 * right13 + left9 * right14 + left13; | ||
| result[14] = left2 * right12 + left6 * right13 + left10 * right14 + left14; | ||
| result[15] = 1.0; | ||
|
|
||
| return result; | ||
| }; | ||
|
|
||
|
|
@@ -2968,25 +2953,23 @@ Matrix4.inverseTranspose = function (matrix, result) { | |
| * @type {Matrix4} | ||
| * @constant | ||
| */ | ||
| Matrix4.IDENTITY = Object.freeze( | ||
| new Matrix4( | ||
| 1.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 1.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 1.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 1.0, | ||
| ), | ||
| Matrix4.IDENTITY = new Matrix4( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. JS does not allow freezing Typed arrays: |
||
| 1.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 1.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 1.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 1.0, | ||
| ); | ||
|
|
||
| /** | ||
|
|
@@ -2995,25 +2978,23 @@ Matrix4.IDENTITY = Object.freeze( | |
| * @type {Matrix4} | ||
| * @constant | ||
| */ | ||
| Matrix4.ZERO = Object.freeze( | ||
| new Matrix4( | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| ), | ||
| Matrix4.ZERO = new Matrix4( | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
| ); | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,7 @@ function Cesium3DTilesetStatistics() { | |
| // Memory statistics | ||
| this.geometryByteLength = 0; | ||
| this.texturesByteLength = 0; | ||
| this.texturesReferenceCounterById = {}; | ||
| this.texturesReferenceCounterById = new Map(); | ||
| this.batchTableByteLength = 0; // batch textures and any binary metadata properties not otherwise accounted for | ||
| } | ||
|
|
||
|
|
@@ -100,12 +100,12 @@ Cesium3DTilesetStatistics.prototype.incrementLoadCounts = function (content) { | |
| const textureIds = content.getTextureIds(); | ||
| for (const textureId of textureIds) { | ||
| const referenceCounter = | ||
| this.texturesReferenceCounterById[textureId] ?? 0; | ||
| this.texturesReferenceCounterById.get(textureId) ?? 0; | ||
| if (referenceCounter === 0) { | ||
| const textureByteLength = content.getTextureByteLengthById(textureId); | ||
| this.texturesByteLength += textureByteLength; | ||
| } | ||
| this.texturesReferenceCounterById[textureId] = referenceCounter + 1; | ||
| this.texturesReferenceCounterById.set(textureId, referenceCounter + 1); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -146,13 +146,13 @@ Cesium3DTilesetStatistics.prototype.decrementLoadCounts = function (content) { | |
| // total textures byte length | ||
| const textureIds = content.getTextureIds(); | ||
| for (const textureId of textureIds) { | ||
| const referenceCounter = this.texturesReferenceCounterById[textureId]; | ||
| const referenceCounter = this.texturesReferenceCounterById.get(textureId); | ||
| if (referenceCounter === 1) { | ||
| delete this.texturesReferenceCounterById[textureId]; | ||
| this.texturesReferenceCounterById.delete(textureId); | ||
| const textureByteLength = content.getTextureByteLengthById(textureId); | ||
| this.texturesByteLength -= textureByteLength; | ||
| } else { | ||
| this.texturesReferenceCounterById[textureId] = referenceCounter - 1; | ||
| this.texturesReferenceCounterById.set(textureId, referenceCounter - 1); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -187,9 +187,7 @@ Cesium3DTilesetStatistics.clone = function (statistics, result) { | |
| statistics.numberOfTilesCulledWithChildrenUnion; | ||
| result.geometryByteLength = statistics.geometryByteLength; | ||
| result.texturesByteLength = statistics.texturesByteLength; | ||
| result.texturesReferenceCounterById = { | ||
| ...statistics.texturesReferenceCounterById, | ||
| }; | ||
| result.texturesReferenceCounterById = statistics.texturesReferenceCounterById; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I did: loaded the basic Performance Profile: Why this change: I audited the code, and saw that while the statistics object is copied frame-to-frame, this property specifically is only ever used by the new statistics object, and passing it by reference kept all the same behavior in prod. Changing it a |
||
| result.batchTableByteLength = statistics.batchTableByteLength; | ||
| }; | ||
| export default Cesium3DTilesetStatistics; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -188,6 +188,11 @@ function isOnScreenLongEnough(tile, frameState) { | |
| * @param {FrameState} frameState | ||
| */ | ||
| Cesium3DTilesetTraversal.updateTile = function (tile, frameState) { | ||
| if (tile._visitedFrame === frameState.frameNumber) { | ||
| // Prevents another pass from visiting the frame again | ||
| return; | ||
|
Comment on lines
+191
to
+193
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tileset statistics are kind of wrong today. Only tiles which are marked as visible and added to the traversal queue are marked as This fixes the above, now all tiles that have they visibility updated are correctly marked as This change shows that on average, there are actually around 60% more visited tiles than previously reported!! |
||
| } | ||
| Cesium3DTilesetTraversal.visitTile(tile, frameState); | ||
| updateTileVisibility(tile, frameState); | ||
| tile.updateExpiration(); | ||
|
|
||
|
|
@@ -279,6 +284,9 @@ function anyChildrenVisible(tile, frameState) { | |
| const child = children[i]; | ||
| child.updateVisibility(frameState); | ||
| anyVisible = anyVisible || child.isVisible; | ||
| if (anyVisible) { | ||
| break; | ||
| } | ||
|
Comment on lines
+287
to
+289
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I did: loaded the basic Performance Profile: I checked in total, what part of the frametime was spent by the 3D Tileset. Between 15-30% of each frame is spent in either of the
Why this change: The performance profile showed that vast majority of time was spent in the tileset traversal updating visibility. I noticed that this method while by name acts like This change results in about 10% fewer tiles visited total (after the change above to make visited tiles more accurate), but I didn't notice a consistent improvement in FPS from this sadly. This change does not from my testing affect the final selected tiles. |
||
| } | ||
| return anyVisible; | ||
| } | ||
|
|
||




There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I did: loaded the basic Google Photorealistic sandcastle and profiled me just moving around slowly
Performance Profile:
I analyzed multiple frames, and saw that
CreditDisplay.endFramewas taking up about 2% of frame time.Why this change:
The performance profile showed that due to the credit clones only passing the original
HTML, every frame for all credits:By passing credits by reference, the steps 1+2 are executed only once when the first credit of that id is added, and then every frame the generated element is reused.
Credits still properly get removed/added when displayed tilesets change. I'm not sure why credits aren't passed by reference, and this isn't a significant performance improvement alone, but together with the others it stacks up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fundamentally changing the behavior of this function to not actually be a
clonelike it's named. I don't think we want to do this. There may be merit in reducing the need to call.clonewherever it's called but a clone function itself should not be returning the original object.I believe @mzschwartz5 was looking into credits recently and might have some more insights. I feel like this is something that could benefit from being managed higher up where we can store the references and reduce the amount
.cloneneeds to be called in the first place.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure, this was more of a POC that passing them by reference does not break rendering them fundamentally. If the team is already looking into this I'll refrain from opening this as a separate PR