-
Notifications
You must be signed in to change notification settings - Fork 191
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
[Feat] - Performance Improvements to GLDraw.ts
, GLCartoon.ts
, GLModel.ts
#662
[Feat] - Performance Improvements to GLDraw.ts
, GLCartoon.ts
, GLModel.ts
#662
Conversation
@dkoes I'm trying to figure out why the CI is failing here, but wasn't able to. Could you please help me with this or suggest what needs to be done? |
Is |
@dkoes Figured it out, it's an issue with |
If the goal is more performant code (great goal!), it would be good to setup a performance benchmarking suite before making changes to make sure improvements are happening. For example, recording the render times of the test suite. |
I think the set() methods are working fine, but its something related to TS and Node/NPM @dkoes The tests are failing on the master branch for me :( Around 18 tests are failing not sure why and this is the error which I am getting for all of them, Edit 1: |
@dkoes I found the issue to be my system itself, its based on arm architecture and node-canvas does not support prebuilt binaries for this architecture. I will continue to update the codebase based on perf improvements. I have a secondary PC which used intel x86_x64 architecture in which I can run the tests and check where it is breaking down :) |
GLDraw.ts
, GLCartoon.ts
, GLModel.ts
Signed-off-by: Adithya Krishna <adikrish@redhat.com>
Signed-off-by: Adithya Krishna <adikrish@redhat.com>
Signed-off-by: Adithya Krishna <adikrish@redhat.com>
Signed-off-by: Adithya Krishna <adikrish@redhat.com>
Signed-off-by: Adithya Krishna <adikrish@redhat.com>
Signed-off-by: Adithya Krishna <adikrish@redhat.com>
Signed-off-by: Adithya Krishna <adikrish@redhat.com>
Signed-off-by: Adithya Krishna <adikrish@redhat.com>
Signed-off-by: Adithya Krishna <adikrish@redhat.com>
37b92d1
to
1c23a8a
Compare
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.
I am never going to approve this PR. There are too many changes and too many of them would introduce regressions (a few minutes looking through several and I only looked at a small fraction of the changes). I suggest starting over. Only make a change if you can prove it is actually an improvement and provide the proof in the PR. There should not be one single "performance improvement" PR, but separate PRs for each issue addressed. (e.g. use of Math.hypot at all relevant places)
var vertexArray = geoGroup.vertexArray; | ||
var colorArray = geoGroup.colorArray; | ||
const vertices = []; | ||
const colors = []; |
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 evidence is there this is a good idea? Instead of directly setting values in an already allocated fixed-size array you are using a second, dynamically resizing list.
if (l > bestlen) { | ||
bestlen = l; | ||
bestv = v; | ||
if (bestlen > 0.1) { |
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.
You've removed the short circuiting of the loop so now it will run longer. Why do you think this is an improvement?
// recast dy in terms of new axes - z is the same | ||
// Using Math.hypot(dx,dy) instead of Math.sqrt(dx * dx + dy * dy) | ||
const dxy = Math.hypot(dx, dy); | ||
const { sin: sinA, cos: cosA } = dxy < 0.0001 ? { sin: 0, cos: 1 } : { sin: -dx / dxy, cos: dy / dxy }; |
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.
Creating an object has more overhead than two simple variables.
rot[6] = sinA * sinB; | ||
rot[7] = -cosA * sinB; | ||
rot[8] = cosB; | ||
const rot = new Float32Array([cosA, sinA, 0, -sinA * cosB, cosA * cosB, sinB, sinA * sinB, -cosA * sinB, cosB]); |
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.
Setting the values individually instead of allocating a list first is more than twice as fast (go ahead and run it through JSBench.Me)
// bottom | ||
nvecs.push(this.basisVectors[i].clone().multiplyScalar(radius)); | ||
nvecs.push(this.basisVectors[i].multiplyScalar(radius)); |
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.
removing the clone will modify the basisVectors
@dkoes I completely understand, I will work on creating seperate PRs for the same. There were a couple of changes I had done, but these were without much understanding of the library. I will close this and create seperate ones as you suggested |
Description: