[devtool] introduce datasink class to etdump #8496

Gasoonjia · 2025-02-14T06:09:34Z

Stack from ghstack (oldest at bottom):

this diff introduce datasink class, the class for managing the customized debug data storage pipeline.

Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0

Differential Revision: D69583422

this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/) [ghstack-poisoned]

pytorch-bot · 2025-02-14T06:09:38Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/8496

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit f2fbce9 with merge base 728c255 ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

facebook-github-bot · 2025-02-14T06:09:46Z

This pull request was exported from Phabricator. Differential Revision: D69583422

this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/) ghstack-source-id: 266441637 Pull Request resolved: #8496

Gasoonjia · 2025-02-14T06:10:37Z

@pytorchbot label "topic: not user facing"

this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/) [ghstack-poisoned]

facebook-github-bot · 2025-02-14T08:35:36Z

This pull request was exported from Phabricator. Differential Revision: D69583422

Pull Request resolved: #8496 this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/) ghstack-source-id: 266455279

dbort

I think that the API should change to take a simple Span<uint8_t> instead of a Tensor; see my comments for details.

devtools/etdump/data_sink.h

devtools/etdump/data_sink_base.h

devtools/etdump/targets.bzl

dbort · 2025-02-14T18:38:41Z

devtools/etdump/data_sink.cpp

+  ET_CHECK_MSG(
+      offset_ <= debug_buffer_.size(),
+      "Ran out of space to store tensor data.");


Please do not use ET_CHECK/ET_CHECK_MSG, which kills the entire process when it fails. This method should return Result<size_t> so that it can fail non-fatally when there's a problem.

dbort · 2025-02-14T18:40:43Z

devtools/etdump/data_sink.cpp

+  }
+  uint8_t* offset_ptr =
+      internal::alignPointer(debug_buffer_.data() + offset_, 64);
+  offset_ = (offset_ptr - debug_buffer_.data()) + tensor.nbytes();


When bumping the offset forward, I strongly suggest zeroing out the memory in the padding. If you don't do that, and this buffer is written to a file, then that file may contain random data from the process.

a) this makes the file contents non-deterministic, and b) it could inadvertently write user data or secrets into the file if they had been stored in the heap and then freed before being used for this buffer.

devtools/etdump/data_sink.cpp

devtools/etdump/data_sink.h

this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/) [ghstack-poisoned]

facebook-github-bot · 2025-02-14T23:03:08Z

This pull request was exported from Phabricator. Differential Revision: D69583422

Pull Request resolved: #8496 this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 ghstack-source-id: 266594898 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/)

this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/) [ghstack-poisoned]

facebook-github-bot · 2025-02-15T02:45:17Z

This pull request was exported from Phabricator. Differential Revision: D69583422

Pull Request resolved: #8496 this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 ghstack-source-id: 266628126 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/)

this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/) [ghstack-poisoned]

facebook-github-bot · 2025-02-15T03:11:43Z

This pull request was exported from Phabricator. Differential Revision: D69583422

Pull Request resolved: #8496 this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 ghstack-source-id: 266630225 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/)

this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/) [ghstack-poisoned]

Pull Request resolved: #8496 this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 ghstack-source-id: 266631718 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/)

facebook-github-bot · 2025-02-15T03:38:45Z

This pull request was exported from Phabricator. Differential Revision: D69583422

this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/) [ghstack-poisoned]

facebook-github-bot · 2025-02-17T23:56:07Z

This pull request was exported from Phabricator. Differential Revision: D69583422

Pull Request resolved: #8496 this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 ghstack-source-id: 266867606 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/)

tarun292 · 2025-02-18T18:57:17Z

Looks great to me overall and i think most of @dbort's comments are addressed now. Will let him take a final look and stamp.

this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/) [ghstack-poisoned]

facebook-github-bot · 2025-02-18T19:41:32Z

This pull request was exported from Phabricator. Differential Revision: D69583422

Pull Request resolved: #8496 this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 ghstack-source-id: 267007242 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/)

dbort

Thanks for renaming the subclass

devtools/etdump/buffer_data_sink.cpp

dbort · 2025-02-19T23:36:03Z

devtools/etdump/buffer_data_sink.cpp

+  if (length == 0) {
+    return static_cast<size_t>(-1);
+  }


From an earlier comment:

@dbort:

size_t is unsigned, so -1 will look like a very large positive number: 18 exabytes on a 64-bit machine. If this is part of the contract, please document it in the base class. But I'd recommend just returning offset_ in this case since it's a zero-sized buffer and the offset doesn't matter.

@Gasoonjia:

let me create another diff for solving that; this function copy&paste from copy_tensor_to_debug_buffer function in ETDumpGen; updating the returning data here may introduce BP issue

What is "BP"? A behavior change in this new function will not affect anyone, because it is not used yet. If some future caller needs this (size_t)-1 behavior for some reason, then they can deal with that themselves: they know when they're writing zero bytes. I'd prefer not to land potentially-dangerous logic like this, because it might never be fixed.

Sry that's a typo. I wanted to type "BC", which refers to "Backward-Compatibility" issue.

This is not a new function. It has existed in the ETDumpGen class (https://github.com/pytorch/executorch/blob/main/devtools/etdump/etdump_flatcc.cpp#L511) for writing tensors into user-owned buffer, and our buffer_data_sink extracted that part of logic into our class for maintaining the current behavior.

Therefore, I would like to reproduce the existing logic first. I know it is not good enough. Will work on it in the following diffs.

But this is a new function that no-one uses yet. If ETDumpGen::copy_tensor_to_debug_buffer() calls this, then you can isolate the old behavior there, like

size_t ETDumpGen::copy_tensor_to_debug_buffer(executorch::aten::Tensor tensor) { // (comment explaining this special case) if (tensor.nbytes() == 0) { return static_cast<size_t>(-1); } return data_sink_->write(tensor.const_data_ptr(), tensor.nbytes()); }

This way, the new method can behave properly from the start, and other users/subclasses of DataSinkBase don't need to duplicate the old behavior.

devtools/etdump/buffer_data_sink.h

devtools/etdump/buffer_data_sink.cpp

devtools/etdump/utils.h

dbort · 2025-02-19T23:53:12Z

devtools/etdump/data_sink_base.h

+      size_t length) = 0;
+  /**
+   * Get the maximum capacity of the debug storage in bytes.
+   * Should return Error::NotSupported if the capacity is not available


If this might return NotSupported, then users of the base class can't depend on it. Ideally, base classes should only contain methods that all subclasses will be able to implement.

I recommend removing this method.

I agree but some of users need to get the size of debug buffer from ETDumpGen (https://github.com/pytorch/executorch/blob/main/devtools/etdump/etdump_flatcc.cpp#L646). Under current datasink scenerio, we need to have a way for retreiving the size of every datasink.

I only see one use of get_debug_buffer_size today, at https://fburl.com/code/3od7ch5t; and that is a bad use of the API. We should talk to them and see if they can do it in a different way, removing the only usage of this method.

we need to have a way for retreiving the size of every datasink

This just isn't possible for all datasinks. If the old ETDump interface needs to support it for now, then we can find a way to isolate that knowledge in the ETDump code, instead of requiring all DataSinks to follow the old pattern.

this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/) [ghstack-poisoned]

facebook-github-bot · 2025-02-20T21:29:25Z

This pull request was exported from Phabricator. Differential Revision: D69583422

Pull Request resolved: #8496 this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 ghstack-source-id: 267495369 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/)

dbort · 2025-02-21T02:31:09Z

devtools/etdump/buffer_data_sink.cpp

+  if (length == 0) {
+    return static_cast<size_t>(-1);
+  }


But this is a new function that no-one uses yet. If ETDumpGen::copy_tensor_to_debug_buffer() calls this, then you can isolate the old behavior there, like

size_t ETDumpGen::copy_tensor_to_debug_buffer(executorch::aten::Tensor tensor) { // (comment explaining this special case) if (tensor.nbytes() == 0) { return static_cast<size_t>(-1); } return data_sink_->write(tensor.const_data_ptr(), tensor.nbytes()); }

This way, the new method can behave properly from the start, and other users/subclasses of DataSinkBase don't need to duplicate the old behavior.

devtools/etdump/buffer_data_sink.cpp

dbort · 2025-02-21T02:35:29Z

devtools/etdump/buffer_data_sink.h

+   * stored.
+   */
+  explicit BufferDataSink(::executorch::runtime::Span<uint8_t> buffer)
+      : debug_buffer_(buffer), offset_(0), alighment_(64) {}


Typo

Suggested change

: debug_buffer_(buffer), offset_(0), alighment_(64) {}

: debug_buffer_(buffer), offset_(0), alignment_(64) {}

dbort · 2025-02-21T02:36:53Z

devtools/etdump/buffer_data_sink.h

+  explicit BufferDataSink(::executorch::runtime::Span<uint8_t> buffer)
+      : debug_buffer_(buffer), offset_(0), alighment_(64) {}


May as well add an alignment param. And please document that it must be a power of 2

Suggested change

explicit BufferDataSink(::executorch::runtime::Span<uint8_t> buffer)

: debug_buffer_(buffer), offset_(0), alighment_(64) {}

explicit BufferDataSink(::executorch::runtime::Span<uint8_t> buffer, size_t alignment = 64)

: debug_buffer_(buffer), offset_(0), alignment_(alignment) {}

dbort · 2025-02-21T02:45:17Z

devtools/etdump/data_sink_base.h

+      size_t length) = 0;
+  /**
+   * Get the maximum capacity of the debug storage in bytes.
+   * Should return Error::NotSupported if the capacity is not available


I only see one use of get_debug_buffer_size today, at https://fburl.com/code/3od7ch5t; and that is a bad use of the API. We should talk to them and see if they can do it in a different way, removing the only usage of this method.

we need to have a way for retreiving the size of every datasink

This just isn't possible for all datasinks. If the old ETDump interface needs to support it for now, then we can find a way to isolate that knowledge in the ETDump code, instead of requiring all DataSinks to follow the old pattern.

devtools/etdump/utils.h

this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/) [ghstack-poisoned]

Pull Request resolved: #8496 this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 ghstack-source-id: 268108451 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/)

facebook-github-bot · 2025-02-24T22:21:58Z

This pull request was exported from Phabricator. Differential Revision: D69583422

dbort

Thank you for all of these changes! A couple minor requests, but this looks great.

devtools/etdump/buffer_data_sink.cpp

devtools/etdump/buffer_data_sink.h

this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/) [ghstack-poisoned]

facebook-github-bot · 2025-02-24T23:06:46Z

This pull request was exported from Phabricator. Differential Revision: D69583422

Pull Request resolved: #8496 this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 ghstack-source-id: 268121454 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/)

this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/) [ghstack-poisoned]

Pull Request resolved: #8496 this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 ghstack-source-id: 268137338 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/)

facebook-github-bot · 2025-02-25T00:11:58Z

This pull request was exported from Phabricator. Differential Revision: D69583422

Pull Request resolved: #8496 this diff introduce datasink class, the class for managing the customized debug data storage pipeline. Detials can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0 ghstack-source-id: 268137338 Differential Revision: [D69583422](https://our.internmc.facebook.com/intern/diff/D69583422/) Co-authored-by: gasoonjia <[email protected]> Co-authored-by: Scott Wolchok <[email protected]>

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 14, 2025

facebook-github-bot added the fb-exported label Feb 14, 2025

pytorch-bot bot added the topic: not user facing label Feb 14, 2025

Gasoonjia mentioned this pull request Feb 14, 2025

[devtool] make ETDumpGen use datasink #8499

Closed

dbort requested changes Feb 14, 2025

View reviewed changes

Gasoonjia mentioned this pull request Feb 17, 2025

[devtool] make datasink as a sepreate directory #8514

Closed

dbort requested changes Feb 19, 2025

View reviewed changes

Gasoonjia requested review from JacobSzwejbka, lucylq and tarun292 as code owners February 20, 2025 21:29

Gasoonjia mentioned this pull request Feb 20, 2025

[devtool] create stream_data_sink #8604

Open

dbort requested changes Feb 21, 2025

View reviewed changes

dbort approved these changes Feb 24, 2025

View reviewed changes

devtools/etdump/buffer_data_sink.cpp Show resolved Hide resolved

devtools/etdump/buffer_data_sink.h Outdated Show resolved Hide resolved

devtools/etdump/buffer_data_sink.h Outdated Show resolved Hide resolved

facebook-github-bot merged commit de29a09 into gh/gasoonjia/1/base Feb 25, 2025
48 of 50 checks passed

facebook-github-bot deleted the gh/gasoonjia/1/head branch February 25, 2025 07:53

facebook-github-bot temporarily deployed to cherry-pick-bot February 25, 2025 07:53 — with GitHub Actions Inactive

pytorchbot mentioned this pull request Feb 25, 2025

[devtool] introduce datasink class to etdump #8677

Merged

	: debug_buffer_(buffer), offset_(0), alighment_(64) {}
	: debug_buffer_(buffer), offset_(0), alignment_(64) {}

		explicit BufferDataSink(::executorch::runtime::Span<uint8_t> buffer)
		: debug_buffer_(buffer), offset_(0), alighment_(64) {}

[devtool] introduce datasink class to etdump #8496

[devtool] introduce datasink class to etdump #8496

Uh oh!

Conversation

Gasoonjia commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/8496

✅ No Failures

Uh oh!

facebook-github-bot commented Feb 14, 2025

Uh oh!

Gasoonjia commented Feb 14, 2025

Uh oh!

facebook-github-bot commented Feb 14, 2025

Uh oh!

dbort left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

facebook-github-bot commented Feb 14, 2025

Uh oh!

facebook-github-bot commented Feb 15, 2025

Uh oh!

facebook-github-bot commented Feb 15, 2025

Uh oh!

facebook-github-bot commented Feb 15, 2025

Uh oh!

facebook-github-bot commented Feb 17, 2025

Uh oh!

tarun292 commented Feb 18, 2025

Uh oh!

facebook-github-bot commented Feb 18, 2025

Uh oh!

dbort left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Feb 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Gasoonjia commented Feb 14, 2025 •

edited

Loading

pytorch-bot bot commented Feb 14, 2025 •

edited

Loading