etdumpfilter impl #9752

Gasoonjia · 2025-03-28T23:15:51Z

Differential Revision: D72025517

pytorch-bot · 2025-03-28T23:15:54Z

🔗 Helpful Links

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

📄 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 b8f8988 with merge base d6e14fc ():
💚 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-03-28T23:16:03Z

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

Gasoonjia · 2025-03-28T23:17:54Z

@pytorchbot label "topic: not user facing"

Summary: Introduce ETDumpFilter for filtering logged delegation intermediate data. Details can be found in pytorch#9260 Differential Revision: D72025517

facebook-github-bot · 2025-03-29T02:30:17Z

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

Summary: Pull Request resolved: pytorch#9752 Introduce ETDumpFilter for filtering logged delegation intermediate data. Details can be found in pytorch#9260 Differential Revision: D72025517

Summary: Introduce ETDumpFilter for filtering logged delegation intermediate data. Details can be found in pytorch#9260 Differential Revision: D72025517

facebook-github-bot · 2025-04-02T09:01:22Z

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

Summary: Pull Request resolved: pytorch#9752 Introduce ETDumpFilter for filtering logged delegation intermediate data. Details can be found in pytorch#9260 Differential Revision: D72025517

swolchok

just some C++ comments; leaving for reviewers with more context on the actual goals here

swolchok · 2025-04-02T18:03:20Z

devtools/etdump/etdump_filter.cpp

+namespace executorch {
+namespace etdump {


we have C++17 -- namespace executorch::etdump {

swolchok · 2025-04-02T18:04:14Z

devtools/etdump/etdump_filter.h

+  size_t regex_count_;
+  size_t range_start_;
+  size_t range_end_;


nit: you don't need the explicit default constructor if you assign 0 to these here; you can then change the constructor implementation in the .cpp file to ETDumpFilter::ETDumpFilter() = default;

swolchok · 2025-04-02T18:05:40Z

devtools/etdump/etdump_filter.cpp

+  if (len >= MAX_PATTERN_LENGTH) {
+    return Error::InvalidArgument; // Pattern too long
+  }
+  strcpy(regex_patterns_[regex_count_], pattern);


nit: you already have the length, so use it -- std::memcpy(regex_patterns_[regex_count_], pattern, len)

swolchok · 2025-04-02T18:06:05Z

devtools/etdump/etdump_filter.cpp

+    return Error::InvalidArgument; // Pattern too long
+  }
+  strcpy(regex_patterns_[regex_count_], pattern);
+  regex_patterns_[regex_count_][len] = '\0';


FYI strcpy already does this. however, if you take my memcpy advice above you'll need to leave it

swolchok · 2025-04-02T18:06:55Z

devtools/etdump/etdump_filter.cpp

+  return true;
+}
+
+Result<bool> ETDumpFilter::set_debug_handle_range(size_t start, size_t end) {


what does a successful result of false mean? (does Result<void> work?)

false will not be use.
We can not use Result<void> cuz Result class need to use reference for the T type and void does not have Reference.

swolchok · 2025-04-02T18:07:10Z

devtools/etdump/etdump_filter.cpp

+
+Result<bool> ETDumpFilter::filter_name_(const char* name) {
+  if (name == nullptr) {
+    return Error::InvalidArgument; // Name is null


comment repeats the code, delete

swolchok · 2025-04-02T18:08:40Z

devtools/etdump/etdump_filter.h

+  size_t get_n_regex() const;
+
+ private:
+  char regex_patterns_[MAX_REGEX_PATTERNS][MAX_PATTERN_LENGTH]{};


why the hardcoded limit on length instead of a std::vector<std::string>? I don't see any particular guarantees from RE2 itself that it doesn't malloc, so isn't studiously avoiding malloc in our wrapper a lost cause?

I don't want to use any memory allocation operation in etdump_filter; our user usually have their own memory allocation way and does not support anything like vector or malloc.

does not support anything like vector or malloc

but that's my point -- RE2 mallocs! (for example https://github.com/google/re2/blob/main/re2/compile.cc#L1254)

ahhh fair point didn't pay attention to RE internal impl

swolchok · 2025-04-02T18:12:06Z

devtools/etdump/etdump_filter.h

+   *         - An error code if number of pattern has reached to cap, or any
+   * error occurs during regex compilation.
+   */
+  Result<bool> add_regex(const char* pattern);


std::string_view is in C++17. why not use it instead? (if the user ever passes a literal, we won't need to check its length at runtime)

thx didn't know we have that; migrating to that.

Summary: Introduce ETDumpFilter for filtering logged delegation intermediate data. Details can be found in pytorch#9260 Differential Revision: D72025517

facebook-github-bot · 2025-04-02T23:01:50Z

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

tarun292 · 2025-04-03T00:48:57Z

devtools/etdump/etdump_filter.cpp

@@ -0,0 +1,100 @@
+/*


Overall high level comment, i think it's reasonable that we can assume the etdump filter we provide based on regex will need malloc to be supported. If users want to use this malloc support will be required on their platform. Based on this i think it would be good to remove the limitations we have in this class such as max regex, max char len etc.

If users do need a filter for their platform that isn't based on regex they can write their own one. (which will be only very few embedded users).

which will be only very few embedded users

ok in this case i agree we can make our etdump_filter support regex + malloc, just as what swolchok mentioned RE use malloc internally. Let me make an update

Summary: Introduce ETDumpFilter for filtering logged delegation intermediate data. Details can be found in pytorch#9260 Differential Revision: D72025517

facebook-github-bot · 2025-04-03T19:26:47Z

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

Summary: Introduce ETDumpFilter for filtering logged delegation intermediate data. Details can be found in pytorch#9260 Reviewed By: tarun292 Differential Revision: D72025517

facebook-github-bot · 2025-04-04T18:05:43Z

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

Summary: Introduce ETDumpFilter for filtering logged delegation intermediate data. Details can be found in pytorch#9260 Reviewed By: tarun292 Differential Revision: D72025517

Summary: Pull Request resolved: pytorch#9752 Introduce ETDumpFilter for filtering logged delegation intermediate data. Details can be found in pytorch#9260 Reviewed By: tarun292 Differential Revision: D72025517

facebook-github-bot · 2025-04-04T20:06:31Z

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

Gasoonjia requested review from JacobSzwejbka, lucylq, swolchok and tarun292 as code owners March 28, 2025 23:15

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 Mar 28, 2025

facebook-github-bot added the fb-exported label Mar 28, 2025

pytorch-bot bot added the topic: not user facing label Mar 28, 2025

Gasoonjia added a commit to Gasoonjia/executorch-1 that referenced this pull request Mar 29, 2025

etdumpfilter impl (pytorch#9752)

53ea16f

Summary: Introduce ETDumpFilter for filtering logged delegation intermediate data. Details can be found in pytorch#9260 Differential Revision: D72025517

Gasoonjia force-pushed the export-D72025517 branch from f0102da to 53ea16f Compare March 29, 2025 02:26

Gasoonjia force-pushed the export-D72025517 branch from 53ea16f to afa0c62 Compare March 29, 2025 02:30

Gasoonjia added a commit to Gasoonjia/executorch-1 that referenced this pull request Apr 2, 2025

etdumpfilter impl (pytorch#9752)

eeaa935

Summary: Introduce ETDumpFilter for filtering logged delegation intermediate data. Details can be found in pytorch#9260 Differential Revision: D72025517

Gasoonjia force-pushed the export-D72025517 branch from afa0c62 to eeaa935 Compare April 2, 2025 08:58

Gasoonjia force-pushed the export-D72025517 branch from eeaa935 to f3bb579 Compare April 2, 2025 09:01

swolchok reviewed Apr 2, 2025

View reviewed changes

Gasoonjia added a commit to Gasoonjia/executorch-1 that referenced this pull request Apr 2, 2025

etdumpfilter impl (pytorch#9752)

92235a1

Summary: Introduce ETDumpFilter for filtering logged delegation intermediate data. Details can be found in pytorch#9260 Differential Revision: D72025517

Gasoonjia force-pushed the export-D72025517 branch from f3bb579 to 92235a1 Compare April 2, 2025 23:01

tarun292 reviewed Apr 3, 2025

View reviewed changes

Gasoonjia force-pushed the export-D72025517 branch from 92235a1 to 379d4e4 Compare April 3, 2025 19:26

Gasoonjia added a commit to Gasoonjia/executorch-1 that referenced this pull request Apr 3, 2025

etdumpfilter impl (pytorch#9752)

379d4e4

Summary: Introduce ETDumpFilter for filtering logged delegation intermediate data. Details can be found in pytorch#9260 Differential Revision: D72025517

tarun292 approved these changes Apr 3, 2025

View reviewed changes

Gasoonjia force-pushed the export-D72025517 branch from 379d4e4 to 504dede Compare April 4, 2025 18:05

Gasoonjia force-pushed the export-D72025517 branch from 504dede to 4b6c76f Compare April 4, 2025 20:02

etdumpfilter impl (pytorch#9752)

b8f8988

Summary: Pull Request resolved: pytorch#9752 Introduce ETDumpFilter for filtering logged delegation intermediate data. Details can be found in pytorch#9260 Reviewed By: tarun292 Differential Revision: D72025517

Gasoonjia force-pushed the export-D72025517 branch from 4b6c76f to b8f8988 Compare April 4, 2025 20:06

facebook-github-bot merged commit 56c8dc2 into pytorch:main Apr 4, 2025
82 of 84 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

etdumpfilter impl #9752

etdumpfilter impl #9752

Gasoonjia commented Mar 28, 2025

pytorch-bot bot commented Mar 28, 2025 •

edited

Loading

facebook-github-bot commented Mar 28, 2025

Gasoonjia commented Mar 28, 2025

facebook-github-bot commented Mar 29, 2025

facebook-github-bot commented Apr 2, 2025

swolchok left a comment

swolchok Apr 2, 2025

swolchok Apr 2, 2025

swolchok Apr 2, 2025

swolchok Apr 2, 2025

swolchok Apr 2, 2025

Gasoonjia Apr 2, 2025

swolchok Apr 2, 2025

swolchok Apr 2, 2025

Gasoonjia Apr 2, 2025

swolchok Apr 2, 2025

Gasoonjia Apr 3, 2025

swolchok Apr 2, 2025

Gasoonjia Apr 2, 2025

facebook-github-bot commented Apr 2, 2025

tarun292 Apr 3, 2025

tarun292 Apr 3, 2025

Gasoonjia Apr 3, 2025 •

edited

Loading

facebook-github-bot commented Apr 3, 2025

facebook-github-bot commented Apr 4, 2025

facebook-github-bot commented Apr 4, 2025

		namespace executorch {
		namespace etdump {

etdumpfilter impl #9752

etdumpfilter impl #9752

Conversation

Gasoonjia commented Mar 28, 2025

pytorch-bot bot commented Mar 28, 2025 • edited Loading

🔗 Helpful Links

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

✅ No Failures

facebook-github-bot commented Mar 28, 2025

Gasoonjia commented Mar 28, 2025

facebook-github-bot commented Mar 29, 2025

facebook-github-bot commented Apr 2, 2025

swolchok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gasoonjia Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Apr 3, 2025

facebook-github-bot commented Apr 4, 2025

facebook-github-bot commented Apr 4, 2025

pytorch-bot bot commented Mar 28, 2025 •

edited

Loading

Gasoonjia Apr 3, 2025 •

edited

Loading