Skip to content

Conversation

@Linchenn
Copy link
Collaborator

@Linchenn Linchenn commented Aug 3, 2020

This PR does the following:

  • add kernelTimeMs (as discussed in this ISSUE) and extraInfo (GPU programs) information into the result of tf.profile()

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@Linchenn Linchenn marked this pull request as ready for review August 3, 2020 23:41
Copy link
Collaborator Author

@Linchenn Linchenn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 approvals obtained


tfjs-core/src/engine.ts, line 649 at r1 (raw file):

        outputShapes: outputs.map(item => item.shape),
        kernelTimeMs: kernelProfile.timeMs,
        extraInfo: kernelProfile.extraInfo

Here, kernelTimeMs and extraInfo are promises. I resolve them at tf.profile(), because the runKernel is not an async function.

Another solution is to resolve them within runKernel by then():

    if (this.state.profiling) {
      Promise.all([kernelProfile.timeMs, kernelProfile.extraInfo]).then(data => {
        this.state.activeProfile.kernels.push({
          name: kernelName,
          ...
          kernelTimeMs: data[0],
          extraInfo: data[1]
        });
      });
    }

Since above code of the second solution is relatively complicate, I chose the first solution.

@Linchenn Linchenn requested review from lina128 and pyu10055 August 4, 2020 15:35
@lina128 lina128 requested a review from annxingyuan August 4, 2020 21:35
Copy link
Contributor

@annxingyuan annxingyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Linchenn this is great! I just left a tiny question regarding one of your comments below - once you address I will approve!

Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan, @lina128, @Linchenn, and @pyu10055)


tfjs-core/src/engine_test.ts, line 399 at r1 (raw file):

    expect(typeof profile.kernels[1].extraInfo).toBe('string');

    // The `kernelTimeMs` and `extraInfo` are tested in Profiler.profileKernel

sorry I'm a little confused about this comment - it looks like kernelTimeMs and extraInfo are tested in the lines above? what do you mean that they are tested in Profiler.profileKernel?

Copy link
Collaborator Author

@Linchenn Linchenn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Ann!

Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan, @lina128, and @pyu10055)


tfjs-core/src/engine_test.ts, line 399 at r1 (raw file):

Previously, annxingyuan (Ann Yuan) wrote…

sorry I'm a little confused about this comment - it looks like kernelTimeMs and extraInfo are tested in the lines above? what do you mean that they are tested in Profiler.profileKernel?

Thanks for pointing it out, and, the comment here is not exact enough. Changed.

The above lines only test whether the promises are resolved, while the values of kernelTimeMs and extraInfo are tested in Profiler.profileKernel's tests.


tfjs-core/src/engine_test.ts, line 401 at r1 (raw file):

    // The `kernelTimeMs` and `extraInfo` are tested in Profiler.profileKernel
    // and are excluded from here.
    expect(profile.kernels[0]).toEqual(jasmine.objectContaining({

I saw our codebase rarely uses jasmine.objectContaining, so do you think it is better to include kernelTimeMs and extraInfo into the object and use toEqual here? like:

    expect(profile.kernels[0]).toEqual({
      'name': 'Square',
      ...
      'outputShapes': [[3]],

      'kernelTimeMs': profile.kernels[0].kernelTimeMs,
      'extraInfo': profile.kernels[0].extraInfo

    });

@Linchenn Linchenn requested a review from annxingyuan August 5, 2020 15:56
Copy link
Contributor

@annxingyuan annxingyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @lina128, @Linchenn, and @pyu10055)


tfjs-core/src/engine_test.ts, line 399 at r1 (raw file):

Previously, Linchenn wrote…

Thanks for pointing it out, and, the comment here is not exact enough. Changed.

The above lines only test whether the promises are resolved, while the values of kernelTimeMs and extraInfo are tested in Profiler.profileKernel's tests.

if you wanted to you could build a stub profiler like we do in profiler_test.ts - otherwise sure asserting that the kernelTimeMs and extraInfo are equal to itself is fine - just add a comment about why we're not asserting that they equal particular values

Copy link
Collaborator Author

@Linchenn Linchenn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @lina128, and @pyu10055)


tfjs-core/src/engine_test.ts, line 399 at r1 (raw file):

Previously, annxingyuan (Ann Yuan) wrote…

if you wanted to you could build a stub profiler like we do in profiler_test.ts - otherwise sure asserting that the kernelTimeMs and extraInfo are equal to itself is fine - just add a comment about why we're not asserting that they equal particular values

Wise points! Thank you so much Ann.

@Linchenn Linchenn merged commit 3d14962 into tensorflow:master Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants