[move] Enable serialization of source maps into json #18727
Merged
Conversation
This file contains 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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
This was referenced Jul 18, 2024
tnowacki
approved these changes
Jul 19, 2024
|
|
||
| pub type Error = (Loc, String); | ||
| pub type Errors = Vec<Error>; | ||
|
|
||
| pub fn source_map_from_file(file_path: &Path) -> Result<SourceMap> { | ||
| if file_path.extension().is_some_and(|ext| ext == "json") { |
There was a problem hiding this comment.
constants are used elsewhere for extension though I understand that json is not like the others.
But it is a fair comment
There was a problem hiding this comment.
My only request is that if we extract this into a constant, that it's the SOURCE_MAP_EXT, and not JSON_EXT.
|
|
||
| pub type Error = (Loc, String); | ||
| pub type Errors = Vec<Error>; | ||
|
|
||
| pub fn source_map_from_file(file_path: &Path) -> Result<SourceMap> { | ||
| if file_path.extension().is_some_and(|ext| ext == "json") { | ||
| return deserialize_from_json(file_path); | ||
| } |
There was a problem hiding this comment.
Should we enforce a file type otherwise? Makes me nervous to just have ".json" or whatever else you want
tzakian
force-pushed
the
tzakian/tracing-1-source-maps-json
branch
from
e704d1d
to
1cf3c88
Compare
tzakian
force-pushed
the
tzakian/tracing-1-source-maps-json
branch
from
1cf3c88
to
5622ad2
Compare
tzakian
force-pushed
the
tzakian/tracing-1-source-maps-json
branch
from
5622ad2
to
e1c998e
Compare
tzakian
force-pushed
the
tzakian/tracing-1-source-maps-json
branch
from
e1c998e
to
61c3ec0
Compare
dariorussi
approved these changes
Sep 1, 2024
|
|
||
| pub type Error = (Loc, String); | ||
| pub type Errors = Vec<Error>; | ||
|
|
||
| pub fn source_map_from_file(file_path: &Path) -> Result<SourceMap> { | ||
| if file_path.extension().is_some_and(|ext| ext == "json") { |
There was a problem hiding this comment.
constants are used elsewhere for extension though I understand that json is not like the others.
But it is a fair comment
tzakian
force-pushed
the
tzakian/tracing-1-source-maps-json
branch
from
61c3ec0
to
dd46d00
Compare
tzakian
force-pushed
the
tzakian/tracing-1-source-maps-json
branch
from
dd46d00
to
44904a8
Compare
tzakian
added a commit
that referenced
this pull request
Sep 11, 2024
…t!) (#18730) ## Description Plumbs tracing into the Move unit tests, and Move CLI (_not_ the Sui CLI just yet!). You can now run with `move test --trace-execution` or `move test --trace-execution <dir>`. If the Move CLI binary was compiled with the `gas-profiled` feature flag enabled this will result in a trace file for each unit test either under `<move-pkg>/traces/` or under `<move-pkg>/<dir>/`. Once we're happy with this, and we've kicked the tires a bit more + moved over test coverage and gas profiling plumbing this up into the Sui CLI is something we should do. This additionally adds a new test to the `./tests.sh` script for external crates to compile and run tracing tests with the `gas-profiler` flag set. ### PR Stack: 1. #18727 2. #18729 3. #18730 **<<<<< You are here** ## Test plan Added a number of tests. --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
tzakian
added a commit
that referenced
this pull request
Sep 11, 2024
## Description This PR adds the initial trace format, and the implementation of this trace format in the VM. I've gone through the tracing format with most of you synchronously so won't write out all the details here (but I will take it on to write a spec for the trace format once this is done so other folks can use it when consuming this format). Happy to go through it at any point in real time. Other TODO: right now the `MoveValue`s only support serialize and not deserialize back into Rust so we can only push a trace out from Rust but not consume it. The next thing I'm working on right now is adding support for that, and it may be either an update to this PR, or an additional PR depending on how involved that is... Tests and integration of this into the Move CLI (not the Sui CLI yet) is in the PR above this one. This keeps the tracing largely feature-gated in the VM, and the only additional overhead/change at runtime with `gas-profiling` turned off is the additional argument, but this argument is unused and should be optimized away (and at worst only add essentially no overhead). I kept the `gas-profiling` feature flag and gated the new tracing under it. The plan being to eventually rename that flag at the same time that we update test coverage and gas profiling to use this new tracing format as well (but that's for a couple future PRs!). ### PR Stack: 1. #18727 2. #18729 **<<<<< You are here** 3. #18730 ## Test plan PR above this one --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
tzakian
added a commit
that referenced
this pull request
Sep 11, 2024
This PR adds the initial trace format, and the implementation of this trace format in the VM. I've gone through the tracing format with most of you synchronously so won't write out all the details here (but I will take it on to write a spec for the trace format once this is done so other folks can use it when consuming this format). Happy to go through it at any point in real time. Other TODO: right now the `MoveValue`s only support serialize and not deserialize back into Rust so we can only push a trace out from Rust but not consume it. The next thing I'm working on right now is adding support for that, and it may be either an update to this PR, or an additional PR depending on how involved that is... Tests and integration of this into the Move CLI (not the Sui CLI yet) is in the PR above this one. This keeps the tracing largely feature-gated in the VM, and the only additional overhead/change at runtime with `gas-profiling` turned off is the additional argument, but this argument is unused and should be optimized away (and at worst only add essentially no overhead). I kept the `gas-profiling` feature flag and gated the new tracing under it. The plan being to eventually rename that flag at the same time that we update test coverage and gas profiling to use this new tracing format as well (but that's for a couple future PRs!). 1. #18727 2. #18729 **<<<<< You are here** 3. #18730 PR above this one --- Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
tzakian
force-pushed
the
tzakian/tracing-1-source-maps-json
branch
from
a6f8f05
to
0cd699e
Compare
This PR adds the initial trace format, and the implementation of this trace format in the VM. I've gone through the tracing format with most of you synchronously so won't write out all the details here (but I will take it on to write a spec for the trace format once this is done so other folks can use it when consuming this format). Happy to go through it at any point in real time. Other TODO: right now the `MoveValue`s only support serialize and not deserialize back into Rust so we can only push a trace out from Rust but not consume it. The next thing I'm working on right now is adding support for that, and it may be either an update to this PR, or an additional PR depending on how involved that is... Tests and integration of this into the Move CLI (not the Sui CLI yet) is in the PR above this one. This keeps the tracing largely feature-gated in the VM, and the only additional overhead/change at runtime with `gas-profiling` turned off is the additional argument, but this argument is unused and should be optimized away (and at worst only add essentially no overhead). I kept the `gas-profiling` feature flag and gated the new tracing under it. The plan being to eventually rename that flag at the same time that we update test coverage and gas profiling to use this new tracing format as well (but that's for a couple future PRs!). 1. #18727 2. #18729 **<<<<< You are here** 3. #18730 PR above this one --- Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
tzakian
force-pushed
the
tzakian/tracing-1-source-maps-json
branch
from
0cd699e
to
2a6ea7d
Compare
tzakian
added a commit
that referenced
this pull request
Sep 13, 2024
suiwombat
pushed a commit
that referenced
this pull request
Sep 16, 2024
## Description Pretty simple -- allows serialization and deserialization of Move source maps into a json format. This is to make it more amenable to being utilized by tools written in languages that don't have access to BCS (e.g., for use alongside tracing). ### PR Stack: 1. #18727 **<<<<< You are here** 2. #18729 3. #18730 ## Test plan CI --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
suiwombat
pushed a commit
that referenced
this pull request
Sep 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Pretty simple -- allows serialization and deserialization of Move source maps into a json format. This is to make it more amenable to being utilized by tools written in languages that don't have access to BCS (e.g., for use alongside tracing).
PR Stack:
Test plan
CI
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.