Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't use a SyntheticProvider for literally every type #133134

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Walnut356
Copy link
Contributor

@Walnut356 Walnut356 commented Nov 17, 2024

Replaces a glob regex with individualized imports for each standard library type. This improves debugger performance by quite a bit when populating lots of values with lots of fields

With the glob, afaik every single value of every single type that the debugger renders is run through a python function that does quite a few string comparisons (i plan to fix those next) to determine the SyntheticProvider to use. It looks like DefaultSyntheticProvider's functions internally call the liblldb c++ functions, which ends up with identical behavior to not using a SyntheticProvider at all, except you have extra python round trips slowing things down.

These sample vidoes were run on x86-64-pc-windows-gnu. vect is a 1000 element Vec<Big>, Big contains a dozen or so Small, and Small contains a dozen or so [i32; 5]

Before:

asdf2.mov

After:

asdf.mov

try-job: aarch64-apple

@rustbot
Copy link
Collaborator

rustbot commented Nov 17, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 17, 2024
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 24, 2024

📌 Commit 673b3d3 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 24, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 25, 2024
…acrum

Don't use a SyntheticProvider for literally every type

Replaces a glob regex with individualized imports for each standard library type. This improves debugger performance by quite a bit when populating lots of values with lots of fields

With the glob, afaik every single value of every single type that the debugger renders is run through a python function that does quite a few string comparisons (i plan to fix those next) to determine the SyntheticProvider to use.  It looks like DefaultSyntheticProvider's functions internally call the liblldb c++ functions, which ends up with identical behavior to not using a SyntheticProvider at all, except you have extra python round trips slowing things down.

These sample vidoes were run on x86-64-pc-windows-gnu. `vect` is a 1000 element `Vec<Big>`, `Big` contains a dozen or so `Small`, and `Small` contains a dozen or so `[i32; 5]`

Before:

https://github.com/user-attachments/assets/07c31fe7-e126-4c2e-8ae9-cfe36e351d3f

After:

https://github.com/user-attachments/assets/6c0d1a45-1ffe-46de-95a0-5dbe59a173b5
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 25, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#132605 (CI: increase timeout from 4h to 6h)
 - rust-lang#133042 (btree: add `{Entry,VacantEntry}::insert_entry`)
 - rust-lang#133070 (Lexer tweaks)
 - rust-lang#133134 (Don't use a SyntheticProvider for literally every type)
 - rust-lang#133411 (the emscripten OS no longer exists on non-wasm targets)
 - rust-lang#133419 (Added a doc test for std::path::strip_prefix)
 - rust-lang#133430 (Tweak parameter mismatch explanation to not say `{unknown}`)
 - rust-lang#133435 (miri: disable test_downgrade_observe test on macOS)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 26, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#132605 (CI: increase timeout from 4h to 6h)
 - rust-lang#133042 (btree: add `{Entry,VacantEntry}::insert_entry`)
 - rust-lang#133070 (Lexer tweaks)
 - rust-lang#133134 (Don't use a SyntheticProvider for literally every type)
 - rust-lang#133411 (the emscripten OS no longer exists on non-wasm targets)
 - rust-lang#133419 (Added a doc test for std::path::strip_prefix)
 - rust-lang#133430 (Tweak parameter mismatch explanation to not say `{unknown}`)
 - rust-lang#133435 (miri: disable test_downgrade_observe test on macOS)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 26, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#132605 (CI: increase timeout from 4h to 6h)
 - rust-lang#133042 (btree: add `{Entry,VacantEntry}::insert_entry`)
 - rust-lang#133070 (Lexer tweaks)
 - rust-lang#133134 (Don't use a SyntheticProvider for literally every type)
 - rust-lang#133411 (the emscripten OS no longer exists on non-wasm targets)
 - rust-lang#133419 (Added a doc test for std::path::strip_prefix)
 - rust-lang#133430 (Tweak parameter mismatch explanation to not say `{unknown}`)
 - rust-lang#133435 (miri: disable test_downgrade_observe test on macOS)

r? `@ghost`
`@rustbot` modify labels: rollup
@jhpratt
Copy link
Member

jhpratt commented Nov 26, 2024

@bors r-

failing in CI: #133442 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 26, 2024
@Walnut356
Copy link
Contributor Author

Ah, well that's an unfortunate issue to say the least. I don't have a great way to run the test suite on my end - I've tried via both x86_64-pc-windows-gnu and x86_64-unknown-linux-gnu (via WSL) and both result in python failing to import the _lldb module for whatever reason.

Based on the output and some manual experimentation, it looks like, by default, lldb uses different formatting defaults for Synthetic vs Non Synthetic types? Essentially the -e option isn't enabled. VSCode + codelldb visualizes it the same either way (including the summary for whatever reason), but the raw string output from LLDB is different.

The result is that the type output with my patch looks like this:

(temp::SomeStruct) *stack_val_ref = (x = 10, y = 23.5)

without the patch, the output (and expectation from the test) is:

(temp::SomeStruct) *stack_val_ref = {
  x = 10
  y = 23.5
}

There is a solution, but it's a little jank.

type summary add -F _ -e -x -h "^.*$" --category Rust

This works (i think) because the summary is invalid, thus it defaults to LLDB's default, but with the extra flags. I'd love to do this in a less stupid way, but as far as I can tell there is no way, given an SBValue, to give that value an SBTypeSummary (and thus set its formatting options such that it prints expanded).

It looks like the order of the commands does matter as well, so it needs to be put before all the rest of the type summary add commands.

With the additional testing, it seems clear that this change mostly impacts codelldb, since codelldb's extra processing steps for synthetic types are relatively slow. That said, it's still probably better overall not to use synthetic types for everything when we don't have to.

I tested this with LLDB-DAP as well and the output seems to be the same before and after

@Walnut356
Copy link
Contributor Author

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 28, 2024
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 28, 2024

📌 Commit 8ecac88 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 28, 2024
Zalathar added a commit to Zalathar/rust that referenced this pull request Nov 29, 2024
…acrum

Don't use a SyntheticProvider for literally every type

Replaces a glob regex with individualized imports for each standard library type. This improves debugger performance by quite a bit when populating lots of values with lots of fields

With the glob, afaik every single value of every single type that the debugger renders is run through a python function that does quite a few string comparisons (i plan to fix those next) to determine the SyntheticProvider to use.  It looks like DefaultSyntheticProvider's functions internally call the liblldb c++ functions, which ends up with identical behavior to not using a SyntheticProvider at all, except you have extra python round trips slowing things down.

These sample vidoes were run on x86-64-pc-windows-gnu. `vect` is a 1000 element `Vec<Big>`, `Big` contains a dozen or so `Small`, and `Small` contains a dozen or so `[i32; 5]`

Before:

https://github.com/user-attachments/assets/07c31fe7-e126-4c2e-8ae9-cfe36e351d3f

After:

https://github.com/user-attachments/assets/6c0d1a45-1ffe-46de-95a0-5dbe59a173b5
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 29, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#131323 (Support `clobber_abi` in AVR inline assembly)
 - rust-lang#133092 (Always set the deployment target when building std)
 - rust-lang#133134 (Don't use a SyntheticProvider for literally every type)
 - rust-lang#133538 (Better diagnostic for fn items in variadic functions)
 - rust-lang#133590 (Rename `-Zparse-only`)
 - rust-lang#133592 (Misc: better instructions for envrc, ignore `/build` instead of `build/`)
 - rust-lang#133608 (Revert rust-lang#133418 (Store coverage source regions as `Span`) due to regression rust-lang#133606)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 29, 2024
…acrum

Don't use a SyntheticProvider for literally every type

Replaces a glob regex with individualized imports for each standard library type. This improves debugger performance by quite a bit when populating lots of values with lots of fields

With the glob, afaik every single value of every single type that the debugger renders is run through a python function that does quite a few string comparisons (i plan to fix those next) to determine the SyntheticProvider to use.  It looks like DefaultSyntheticProvider's functions internally call the liblldb c++ functions, which ends up with identical behavior to not using a SyntheticProvider at all, except you have extra python round trips slowing things down.

These sample vidoes were run on x86-64-pc-windows-gnu. `vect` is a 1000 element `Vec<Big>`, `Big` contains a dozen or so `Small`, and `Small` contains a dozen or so `[i32; 5]`

Before:

https://github.com/user-attachments/assets/07c31fe7-e126-4c2e-8ae9-cfe36e351d3f

After:

https://github.com/user-attachments/assets/6c0d1a45-1ffe-46de-95a0-5dbe59a173b5
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 29, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#132782 (improvements on initial sysroot and libdir finding logics)
 - rust-lang#133134 (Don't use a SyntheticProvider for literally every type)
 - rust-lang#133466 (Fix typos in pin.rs)
 - rust-lang#133492 (bootstrap: allow skipping steps with start of path)
 - rust-lang#133501 (support revealing defined opaque post borrowck)
 - rust-lang#133530 (Use consistent wording in docs, use is zero instead of is 0)
 - rust-lang#133538 (Better diagnostic for fn items in variadic functions)
 - rust-lang#133590 (Rename `-Zparse-only`)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member

@bors r- rollup=never
#133612 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 29, 2024
@Walnut356
Copy link
Contributor Author

Whoops, looks like there was never any explicit type synthetic lookup for tuples, they just happened to be caught by the wildcard. Every case that failed was one that involved tuples. Should be all good now

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 30, 2024
@Zalathar
Copy link
Contributor

@bors try

@bors
Copy link
Contributor

bors commented Nov 30, 2024

⌛ Trying commit 9b4a247 with merge de6acf0...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 30, 2024
Don't use a SyntheticProvider for literally every type

Replaces a glob regex with individualized imports for each standard library type. This improves debugger performance by quite a bit when populating lots of values with lots of fields

With the glob, afaik every single value of every single type that the debugger renders is run through a python function that does quite a few string comparisons (i plan to fix those next) to determine the SyntheticProvider to use.  It looks like DefaultSyntheticProvider's functions internally call the liblldb c++ functions, which ends up with identical behavior to not using a SyntheticProvider at all, except you have extra python round trips slowing things down.

These sample vidoes were run on x86-64-pc-windows-gnu. `vect` is a 1000 element `Vec<Big>`, `Big` contains a dozen or so `Small`, and `Small` contains a dozen or so `[i32; 5]`

Before:

https://github.com/user-attachments/assets/07c31fe7-e126-4c2e-8ae9-cfe36e351d3f

After:

https://github.com/user-attachments/assets/6c0d1a45-1ffe-46de-95a0-5dbe59a173b5

---

try-job: aarch64-apple
@bors
Copy link
Contributor

bors commented Nov 30, 2024

☀️ Try build successful - checks-actions
Build commit: de6acf0 (de6acf077184c7e69e73ea3c456ecf48cbd667a1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants