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

Update benchmarks to use EXT_disjoint_query_timer where available. #140

Merged
merged 5 commits into from
Sep 21, 2017

Conversation

nsthorat
Copy link
Contributor

@nsthorat nsthorat commented Sep 20, 2017

This change also adds an Environment class with ENV global representing Feature flags that get lazily evaluated and cached.

This change is Reviewable

@dsmilkov
Copy link
Contributor

:lgtm_strong:


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


demos/benchmarks/conv_benchmarks.ts, line 41 at r1 (raw file):

Promise

Can you verify this works on Safari Desktop (just wondering if Promise needs a polyfill for safari)


src/environment_test.ts, line 1 at r1 (raw file):

remove leading new line


src/util.ts, line 225 at r1 (raw file):

}

export function tryWithBackoff(

couple unit tests for this?


src/math/webgl/gpgpu_context.ts, line 285 at r1 (raw file):

() => {

nit: might be easier to read if you assign the function in a separate value:

const queryGpu = () => { ...}
util.tryWithBackoff(queryGpu, [0, 100, 1000])


src/math/webgl/gpgpu_context.ts, line 298 at r1 (raw file):

0, 100, 1000]

Benchmarks might take significantly more time on older iPhones/Androis. Maybe add the option for tryWithBackoff to wait indefinitely, or even better change the api where the user just provides the maxWaitTime (instead of list of times) and do exponential backoff as an implementation detail?


Comments from Reviewable

@nsthorat
Copy link
Contributor Author

Review status: 1 of 20 files reviewed at latest revision, 5 unresolved discussions, some commit checks pending.


demos/benchmarks/conv_benchmarks.ts, line 41 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Promise

Can you verify this works on Safari Desktop (just wondering if Promise needs a polyfill for safari)

Spoke offline - we have promises all over the place, so we'll have to fix this at a higher level.


src/environment_test.ts, line 1 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

remove leading new line

Done.


src/math/webgl/gpgpu_context.ts, line 285 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

() => {

nit: might be easier to read if you assign the function in a separate value:

const queryGpu = () => { ...}
util.tryWithBackoff(queryGpu, [0, 100, 1000])

Done.


src/math/webgl/gpgpu_context.ts, line 298 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

0, 100, 1000]

Benchmarks might take significantly more time on older iPhones/Androis. Maybe add the option for tryWithBackoff to wait indefinitely, or even better change the api where the user just provides the maxWaitTime (instead of list of times) and do exponential backoff as an implementation detail?

Done.


Comments from Reviewable

@nsthorat
Copy link
Contributor Author

Done!


Review status: 1 of 20 files reviewed at latest revision, 5 unresolved discussions, some commit checks pending.


src/util.ts, line 225 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

couple unit tests for this?

Done.


Comments from Reviewable

@nsthorat nsthorat merged commit 7e72534 into master Sep 21, 2017
@dsmilkov dsmilkov deleted the query-timer branch September 28, 2017 15:10
mnottheone pushed a commit to mnottheone/deeplearnjs that referenced this pull request Dec 1, 2018
…ensorflow#140)

* Update benchmarks to use disjoint query timer

* unit tests for webgl

* respond to comments, add url parsing

* merge

* reduction / unary update to use disjoint query timer
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