Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

Conversation

@dsmilkov
Copy link
Contributor

@dsmilkov dsmilkov commented Oct 3, 2017

This has performance impact all across the board


This change is Reviewable

@dsmilkov dsmilkov requested a review from nsthorat October 3, 2017 22:16
@nsthorat
Copy link
Contributor

nsthorat commented Oct 3, 2017

:lgtm_strong:


Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


src/math/webgl/gpgpu_math.ts, line 73 at r1 (raw file):

  }
  const attributeLocations: {[name: string]: number} = {};
  attributeLocations['uv'] = gpgpu.getAttributeLocation(webGLProgram, 'uv');

can you pull this out so we dont repeat?

const attributeNames = ['uv', 'clipSpacePos'];

and just iterate and collect the locations?


Comments from Reviewable

@dsmilkov
Copy link
Contributor Author

dsmilkov commented Oct 3, 2017

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


src/math/webgl/gpgpu_math.ts, line 73 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

can you pull this out so we dont repeat?

const attributeNames = ['uv', 'clipSpacePos'];

and just iterate and collect the locations?

Done.


Comments from Reviewable

@dsmilkov dsmilkov merged commit 5808682 into master Oct 3, 2017
@dsmilkov dsmilkov deleted the cache-attributes branch October 3, 2017 22:25
mnottheone pushed a commit to mnottheone/deeplearnjs that referenced this pull request Dec 1, 2018
It's a good idea to join the mailing list to discuss work before submitting a PR, and check out the help wanted issues for tasks.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants