[DebuggerV2] Use more efficient read in /execution/data and /graph_execution/data routes #3526
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wchargin
reviewed
Apr 18, 2020
There was a problem hiding this comment.
This is replacing a single query with a classic “n + 1 queries”
problem. We’ve just gone to a bunch of work to remove query patterns
like this from the hparams plugin. Can we instead make the reader accept
a begin/end step range so that you only need one query that returns
exactly what’s needed?
caisq
changed the title
|
@wchargin Thanks for the review and comment. I've updated this PR. The underlying methods of PTAL. |
caisq
added a commit
to caisq/tensorboard
that referenced
this pull request
May 19, 2020
…ecution/data routes (tensorflow#3526) * Motivation for features / changes * Fix a performance issue in the HTTP route /graph_execution/data * The issue has to do with the fact that `DebugDataMultiplexer.GraphExecutionData()` uses the inefficient approach of reading all the `GraphExecutionTrace`s first and then slice the array. A much more efficient approach (analogous to the `DebugDataMultiplexer.ExecutionData()` for top-level executions) is to use the range reading support of `DebugDatataReader.graph_execution_traces()` that is now available. * Similarly, use the range reading support of `DebugDatataReader.execution()` in the `/execution/data` route. * Technical description of changes * See the description above. Use range reading support of `DebugDatataReader.execution()` and `DebugDatataReader.graph_execution_trace()`. * Also, remove an extraneous element in the json response (`num_digests`). It is not in the design spec and was included due to oversight on my part. * Detailed steps to verify changes work correctly (as executed by you) * No new unit tests are required to verify the correctness of the new implementation. The existing ones suffice. * Manual testing in the web GUI to confirm much more efficient loading.
caisq
added a commit
that referenced
this pull request
May 27, 2020
…ecution/data routes (#3526) * Motivation for features / changes * Fix a performance issue in the HTTP route /graph_execution/data * The issue has to do with the fact that `DebugDataMultiplexer.GraphExecutionData()` uses the inefficient approach of reading all the `GraphExecutionTrace`s first and then slice the array. A much more efficient approach (analogous to the `DebugDataMultiplexer.ExecutionData()` for top-level executions) is to use the range reading support of `DebugDatataReader.graph_execution_traces()` that is now available. * Similarly, use the range reading support of `DebugDatataReader.execution()` in the `/execution/data` route. * Technical description of changes * See the description above. Use range reading support of `DebugDatataReader.execution()` and `DebugDatataReader.graph_execution_trace()`. * Also, remove an extraneous element in the json response (`num_digests`). It is not in the design spec and was included due to oversight on my part. * Detailed steps to verify changes work correctly (as executed by you) * No new unit tests are required to verify the correctness of the new implementation. The existing ones suffice. * Manual testing in the web GUI to confirm much more efficient loading.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DebugDataMultiplexer.GraphExecutionData()uses the inefficient approach of reading all theGraphExecutionTraces first and then slice the array. A much more efficient approach (analogous to theDebugDataMultiplexer.ExecutionData()for top-level executions) is to use the range reading support ofDebugDatataReader.graph_execution_traces()that is now available.DebugDatataReader.execution()in the/execution/dataroute.DebugDatataReader.execution()andDebugDatataReader.graph_execution_trace().num_digests). It is not in the design spec and was included due to oversight on my part.