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 Sep 5, 2017

This change is Reviewable

@dsmilkov dsmilkov requested a review from nsthorat September 5, 2017 01:15
@dsmilkov
Copy link
Contributor Author

dsmilkov commented Sep 5, 2017

Also tested model-builder and ran benchmark (no perf degrade, actually shaders run slightly faster ~2%)

@dsmilkov
Copy link
Contributor Author

dsmilkov commented Sep 5, 2017

Same PR includes profiling functionality in the console when math.enableDebugMode() is called.

screen shot 2017-09-04 at 7 56 25 pm

@nsthorat
Copy link
Contributor

nsthorat commented Sep 5, 2017

:lgtm_strong:


Reviewed 23 of 23 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


src/math/math.ts, line 231 at r1 (raw file):

    let start: number;
    if (this.debugMode) {
      start = Date.now();

use performance.now()


src/math/webgl/gpgpu_context_test.ts, line 48 at r1 (raw file):

    const result = gpgpu.downloadMatrixFromTexture(texture, 1, 1);
    expect(result[0]).toBeCloseTo(1.234);
  });

add the same test here


Comments from Reviewable

@dsmilkov
Copy link
Contributor Author

dsmilkov commented Sep 6, 2017

Review status: 21 of 23 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


src/math/math.ts, line 231 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

use performance.now()

Done.


src/math/webgl/gpgpu_context_test.ts, line 48 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

add the same test here

Done.


Comments from Reviewable

@dsmilkov dsmilkov merged commit 25f8967 into master Sep 6, 2017
@dsmilkov dsmilkov deleted the int_indexing branch September 7, 2017 19:52
mnottheone pushed a commit to mnottheone/deeplearnjs that referenced this pull request Dec 1, 2018
* switch shader indexing from float to int

* revert graph_runner_test

* self review
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