Add `SourceView` which doesn't own source text as base class of `Source` #65309

gmagogsfm · 2021-09-19T07:49:24Z

This would save the cost copying text from stack to heap in some cases (like
parsing function schema during loading phase of libtorch.so)

facebook-github-bot · 2021-09-19T07:49:30Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/65309
📄 Preview docs built from this PR
📄 Preview C++ docs built from this PR
🔧 Opt-in to CIFlow to control what jobs run on your PRs

💊 CI failures summary and remediations

As of commit e3e35f5 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

facebook-github-bot · 2021-09-20T17:53:02Z

@gmagogsfm has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-09-20T17:57:24Z

@gmagogsfm has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

suo · 2021-09-20T19:19:52Z

torch/csrc/jit/frontend/source_range.h

@@ -13,7 +13,8 @@ class SourceRangeUnpickler;
 struct SourceRange;

 // Source represents a code segment. It keeps track of:
-//  - text : the text of the code segment
+//  - text or text_view : the text of the code segment, or a view into it


mmm this makes ownership a little tangled, right? We do not really enforce that the source text sticks around, which makes this change unsafe in general; I don't really love "maybe-owned" style classes when they're not absolutely necessary.

Possibly: we could create a SourceView class that shares most implementation with Source but operates over a view, and use that during the startup schema parsing.

If that's too hard we definitely could leave as is and just say it's up to the caller to keep things straight, but ideally we can be more explicit.

I don't really love "maybe-owned" style classes when they're not absolutely necessary.

I would support enabling c10::MaybeOwned<std::string> where the borrow type is c10::string_view if this turns out to be a good use case for it!

facebook-github-bot · 2021-09-20T22:57:28Z

@gmagogsfm has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

swolchok · 2021-09-20T22:59:41Z

torch/csrc/jit/frontend/source_range.h

@@ -105,15 +113,41 @@ struct Source {
  std::shared_ptr<SourceRangeUnpickler> gen_ranges_;
 };

-// A SourceRange is a view into a Source, that points to a subset of the source,
-// specified by `start` and `end` byte offsets into the source text.
+struct SourceView : public Source {


I don't think this is a particularly good use of inheritance. A Source owns its string whereas a SourceView does not; handing a SourceView to code that expects a Source could easily end in tears. CC @ezyang for the Liskov Substitution Principle lecture :)

Yeah, that's a real possibility but I think it should be fine in this case because the derived class already clearly states that it is a view and does not own the text. Thus caller is responsible for making the lifetime correct. Let me know what you think.

The derived class already clearly states that it is a view

Code written to expect a Source knows nothing about SourceView and thus can't see those statements. It may, for example, try to retain a Source for later use. Inheritance should not be used merely for implementation reuse.

Sorry for the delay. I have been on PTO recently.

What if we refactor code to make Source and SourceView both inherit from SourceBase, which makes no guarantee about ownership?

So, typically, when you have a SourceView object and a Source object, you write all clients with SourceView and you make Source implicitly convertible to SourceView. Does this work here?

Sounds like a good plan, making sure we are all on the same page:

SourceView inherits from Source, overriding its text method, allowing implicit construction of SourceView from Source

All current clients of Source should use SourceView, except for ErrorReport, which should always retain a real copy of source text.

Producers of Source stay unchanged.

@swolchok @suo sounds good?

SourceView inherits from Source, overriding its text method, allowing implicit construction of SourceView from Source

Inheriting like that won't allow implicit construction of SourceView from Source, it would allow implicit construction of Source from SourceView.

facebook-github-bot · 2021-09-21T06:02:04Z

@gmagogsfm has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

codecov · 2021-09-21T10:46:31Z

Codecov Report

Merging #65309 (729ee5a) into master (feefc94) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 729ee5a differs from pull request most recent head e3e35f5. Consider uploading reports for the commit e3e35f5 to get more accurate results

@@            Coverage Diff             @@
##           master   #65309      +/-   ##
==========================================
- Coverage   66.37%   66.37%   -0.01%     
==========================================
  Files         739      738       -1     
  Lines       94299    94158     -141     
==========================================
- Hits        62595    62497      -98     
+ Misses      31704    31661      -43

swolchok · 2021-09-23T20:08:39Z

torch/csrc/jit/frontend/function_schema_parser.cpp

@@ -27,7 +27,7 @@ namespace jit {
 namespace {
 struct SchemaParser {
  SchemaParser(const std::string& str)
-      : L(std::make_shared<Source>(str)),
+      : L(std::make_shared<SourceView>(c10::string_view(str))),


why is it necessary to heap-allocate and reference-count the SourceView instead of storing it by value?

Not exactly necessary, but changing this would be pretty involved (other structures like SourceRange, importer etc all use shared_ptr), leaving it as a future todo.

swolchok · 2021-09-23T20:09:48Z

torch/csrc/jit/python/python_tree_views.cpp

@@ -104,7 +104,9 @@ void initTreeViewBindings(PyObject* module) {
            return SourceRange(self.source_, start, end);
          })
      .def_property_readonly("source", [](const SourceRangeFactory& self) {
-        return self.source_->text();
+        auto text_view = self.source_->text();
+        std::string text = std::string(text_view.begin(), text_view.end());


nit: this can be more succinctly written as std::string text(text_view.begin(), text_view.end());

sounds good, done.

pytorch-probot · 2021-10-05T21:17:00Z

CI Flow Status

⚛️ CI Flow

Ruleset - Version: v1
Ruleset - File: https://github.com/gmagogsfm/pytorch-1/blob/e3e35f579f067dfee8672a556353f192fcd73555/.github/generated-ciflow-ruleset.json
PR ciflow labels: ciflow/default

Workflows	Labels (bold enabled)	Status
Triggered Workflows
linux-bionic-py3.6-clang9	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/noarch`, `ciflow/xla`	✅ triggered
linux-vulkan-bionic-py3.6-clang9	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/vulkan`	✅ triggered
linux-xenial-cuda11.3-py3.6-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/default`, `ciflow/linux`	✅ triggered
linux-xenial-py3.6-clang7-asan	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/sanitizers`	✅ triggered
linux-xenial-py3.6-clang7-onnx	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/onnx`	✅ triggered
linux-xenial-py3.6-gcc5.4	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`	✅ triggered
linux-xenial-py3.6-gcc7-bazel-test	`ciflow/all`, `ciflow/bazel`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`	✅ triggered
win-vs2019-cpu-py3	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/win`	✅ triggered
win-vs2019-cuda11.3-py3	`ciflow/all`, `ciflow/cuda`, `ciflow/default`, `ciflow/win`	✅ triggered
Skipped Workflows
libtorch-linux-xenial-cuda10.2-py3.6-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/libtorch`, `ciflow/linux`	🚫 skipped
libtorch-linux-xenial-cuda11.3-py3.6-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/libtorch`, `ciflow/linux`	🚫 skipped
linux-bionic-cuda10.2-py3.9-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/linux`, `ciflow/slow`	🚫 skipped
linux-xenial-cuda10.2-py3.6-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/linux`, `ciflow/slow`	🚫 skipped
parallelnative-linux-xenial-py3.6-gcc5.4	`ciflow/all`, `ciflow/cpu`, `ciflow/linux`	🚫 skipped
periodic-libtorch-linux-xenial-cuda11.1-py3.6-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/libtorch`, `ciflow/linux`, `ciflow/scheduled`	🚫 skipped
periodic-linux-xenial-cuda10.2-py3-gcc7-slow-gradcheck	`ciflow/all`, `ciflow/cuda`, `ciflow/linux`, `ciflow/scheduled`, `ciflow/slow`, `ciflow/slow-gradcheck`	🚫 skipped
periodic-linux-xenial-cuda11.1-py3.6-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/linux`, `ciflow/scheduled`	🚫 skipped
periodic-win-vs2019-cuda11.1-py3	`ciflow/all`, `ciflow/cuda`, `ciflow/scheduled`, `ciflow/win`	🚫 skipped
puretorch-linux-xenial-py3.6-gcc5.4	`ciflow/all`, `ciflow/cpu`, `ciflow/linux`	🚫 skipped

You can add a comment to the PR and tag @pytorchbot with the following commands:

# ciflow rerun, "ciflow/default" will always be added automatically
@pytorchbot ciflow rerun

# ciflow rerun with additional labels "-l <ciflow/label_name>", which is equivalent to adding these labels manually and trigger the rerun
@pytorchbot ciflow rerun -l ciflow/scheduled -l ciflow/slow

For more information, please take a look at the CI Flow Wiki.

facebook-github-bot · 2021-10-06T06:27:06Z

@gmagogsfm has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-10-13T00:31:10Z

@gmagogsfm has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-10-15T21:31:23Z

@gmagogsfm has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-10-15T21:31:39Z

@gmagogsfm has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

instead. This would save the cost copying text from stack to heap in some cases (like parsing function schema during loading phase of libtorch.so)

facebook-github-bot · 2021-10-15T23:56:30Z

@gmagogsfm has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

swolchok

well done!

swolchok · 2021-10-18T23:23:05Z

torch/csrc/jit/frontend/source_range.h

  size_t start_;
  size_t end_;
 };

+// OwnedSourceRange is just like a SourceRange except that it owns a `Source`
+// instead of `SourceView`. Thus OwnedSourceRange owns a copy of source text.
+struct OwnedSourceRange : public SourceRange {


If practical, I would recommend disabling copy & move on SourceRange to avoid accidentally slicing an OwnedSourceRange.

swolchok · 2021-10-18T23:23:46Z

torch/csrc/jit/ir/ir.cpp

@@ -1641,7 +1641,7 @@ size_t Node::blocksFromGraphBlock() {
 }

 inline const SourceRange& fakeRange() {
-  static SourceRange range(std::make_shared<Source>(""), 0, 1);
+  static SourceRange range(std::make_shared<Source>(std::string("")), 0, 1);


just std::string() is slightly more efficient IIRC and it is certainly less for the compiler to figure out

swolchok · 2021-10-18T23:24:20Z

torch/csrc/jit/python/python_tree_views.cpp

+        std::string text(text_view.begin(), text_view.end());
+        return text;


return directly to avoid an extra copy

return std::string(text_view.begin(), text_view.end());

…ce` (#65309) Summary: This would save the cost copying text from stack to heap in some cases (like parsing function schema during loading phase of libtorch.so) Pull Request resolved: #65309 Reviewed By: swolchok Differential Revision: D31060315 Pulled By: gmagogsfm fbshipit-source-id: 0caf7a688b40df52bb4388c5191d1a42351d6f1a

facebook-github-bot added oncall: jit Add this issue/PR to JIT oncall triage queue cla signed labels Sep 19, 2021

gmagogsfm force-pushed the lazy_source_new branch 4 times, most recently from eeaf99d to 352fbbc Compare September 20, 2021 17:52

gmagogsfm force-pushed the lazy_source_new branch from 352fbbc to 29c27c3 Compare September 20, 2021 17:56

gmagogsfm marked this pull request as ready for review September 20, 2021 19:11

gmagogsfm requested review from suo and swolchok September 20, 2021 19:11

suo requested changes Sep 20, 2021

View reviewed changes

gmagogsfm force-pushed the lazy_source_new branch from 29c27c3 to 15d061f Compare September 20, 2021 22:56

swolchok reviewed Sep 20, 2021

View reviewed changes

gmagogsfm force-pushed the lazy_source_new branch from 15d061f to 729ee5a Compare September 21, 2021 05:58

swolchok reviewed Sep 23, 2021

View reviewed changes

gmagogsfm force-pushed the lazy_source_new branch from 729ee5a to 76a7d82 Compare October 5, 2021 21:16

pytorch-probot bot added the ciflow/default label Oct 5, 2021

gmagogsfm force-pushed the lazy_source_new branch 2 times, most recently from 716ea3e to c8ad5f4 Compare October 6, 2021 06:24

gmagogsfm force-pushed the lazy_source_new branch from c8ad5f4 to f705f10 Compare October 6, 2021 19:19

gmagogsfm requested review from suo and swolchok October 6, 2021 22:47

gmagogsfm changed the title ~~Allow Source to not own source code text and operate on string_view~~ Add SourceView which doesn't own source text as base class of Source Oct 6, 2021

gmagogsfm force-pushed the lazy_source_new branch from f705f10 to 6854f81 Compare October 13, 2021 00:30

gmagogsfm force-pushed the lazy_source_new branch 3 times, most recently from 97150bd to d1ef55a Compare October 15, 2021 21:31

gmagogsfm force-pushed the lazy_source_new branch from d1ef55a to 1a54437 Compare October 15, 2021 23:42

Allow Source to not own source code text and operate on string_view

e3e35f5

instead. This would save the cost copying text from stack to heap in some cases (like parsing function schema during loading phase of libtorch.so)

gmagogsfm force-pushed the lazy_source_new branch from 1a54437 to e3e35f5 Compare October 15, 2021 23:53

gmagogsfm requested a review from zhxchen17 October 16, 2021 05:48

swolchok approved these changes Oct 18, 2021

View reviewed changes

facebook-github-bot closed this Oct 19, 2021

		std::string text(text_view.begin(), text_view.end());
		return text;

Add SourceView which doesn't own source text as base class of Source #65309

Add SourceView which doesn't own source text as base class of Source #65309

Uh oh!

Conversation

gmagogsfm commented Sep 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Sep 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

💊 CI failures summary and remediations

Uh oh!

facebook-github-bot commented Sep 20, 2021

Uh oh!

facebook-github-bot commented Sep 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 20, 2021

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 21, 2021

Uh oh!

codecov bot commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

Choose a reason for hiding this comment

Uh oh!

pytorch-probot bot commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚛️ CI Flow

Uh oh!

facebook-github-bot commented Oct 6, 2021

Uh oh!

facebook-github-bot commented Oct 13, 2021

Uh oh!

facebook-github-bot commented Oct 15, 2021

Uh oh!

facebook-github-bot commented Oct 15, 2021

Uh oh!

facebook-github-bot commented Oct 15, 2021

Uh oh!

swolchok left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Add `SourceView` which doesn't own source text as base class of `Source` #65309

Add `SourceView` which doesn't own source text as base class of `Source` #65309

gmagogsfm commented Sep 19, 2021 •

edited

Loading

facebook-github-bot commented Sep 19, 2021 •

edited

Loading

codecov bot commented Sep 21, 2021 •

edited

Loading

pytorch-probot bot commented Oct 5, 2021 •

edited

Loading