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

Continue on with error-recovery infra refresh #1649

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

wycats
Copy link
Contributor

@wycats wycats commented Nov 3, 2024

This PR focuses on bringing in the improvements to local logging and debug infrastructure.

The code is a little bit elaborate, but it does a good job of making the debug trace logs for VM operations clear and comprehensible.

Here are some examples of improved output:

image

image

Notable:

  • The overall output is significantly clearer, with a decent amount of effort placed into turning internal representations into representations that would be useful to people working on the VM.
  • Objects are represented inline as footnotes, and then logged as normal objects on a following line
  • Format specifiers (%d, %o, etc.) are used for inline values with a built-in inline representation
  • The trace logs now report changes to the VM state, rather than the entire VM state each time.
  • Array changes are represented in terms of ...x items followed by a representation of push and pop operations.

In addition, it's possible to enable "subtle" mode, which shows all values rather than just changes.

image

Unchanged values are generally greyed out, so turning on subtle mode can be useful to get the full VM state for a change in normal trace logging mode, while still retaining the ability to identify changes.

Internally, there are two parts of this change:

  1. An internal refactor to the debug state used to emit the opcode logs so that they're more consistent and more consistently snapshottable.
  2. An internal, trace-only API for creating loggable fragments with support for subtle variations. These fragments can be combined naturally using template strings and combinators (e.g. join to join a list of fragments with a separator). When combined, the new fragment has natural subtle behavior (e.g. a list made entirely of subtle values joined with , is subtle).

@wycats wycats force-pushed the feature/error-recovery-redux-pt3 branch from 4127f11 to 22e4216 Compare November 6, 2024 22:44
@wycats wycats force-pushed the feature/error-recovery-redux-pt3 branch from 94e4b50 to 82e3457 Compare November 8, 2024 00:16
@wycats wycats added the perf label Nov 8, 2024
@wycats wycats force-pushed the feature/error-recovery-redux-pt3 branch from b79845d to a9717f3 Compare November 9, 2024 03:59
@wycats wycats force-pushed the feature/error-recovery-redux-pt3 branch from 0fde5c8 to f0a6f79 Compare November 19, 2024 01:29
Move curried types to constants

Improvements to the local debug output

Further clean up metadata and fragment infra

Improve symbols

- Test `@names` in the debugger
- Consolidate symbol names in multiple places
- Rename "eval" names (a holdover from the partial days) to "debug"
  names.

Future work could include further consolidating symbol names to
rationalize lexical scope so that lexical names work in the debugger.

Trace log cleanup

This commit is mostly focused on changing the internals so that we can
create complete trace logs without having to pass in the entire VM.

Mostly finished implementing improved logging.

More trace log improvements

Fix lints
@wycats wycats force-pushed the feature/error-recovery-redux-pt3 branch from f0a6f79 to 35817f7 Compare November 19, 2024 02:09
@wycats
Copy link
Contributor Author

wycats commented Nov 19, 2024

There are still a number of places that could benefit from further improvements to their trace log representation, but this PR creates a strong foundation to build on and is, itself a significant improvement to the status quo.

I ran the benchmarks locally and this PR didn't affect performance. This is to be expected since virtually all of the changes happened inside of LOCAL_DEBUG checks.

@NullVoxPopuli
Copy link
Contributor

perf test shows no difference 🎉


function describeBlock(
block: AppendingBlock,
type: 'block:simple' | 'block:remote' | 'block:resettable'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simple blocks were renamed to appending blocks, yeah?

what determines simple vs remote vs resettable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Appending: A block that will never be cleared. However, since it's possible to append a block to an Appending Block, the boundaries of an Appending Block can change. See FirstNode / LastNode for details on how this works.
  • Resettable: A block that can be reset (i.e. cleared). When a block is reset, its contents are cleared and it produces a new cursor to render a block into. This is used for constructs like {{#if}} / {{else}}, which clear and rerender the block whenever the boolean value changes.
  • Remote: A block that represents a remote element. When the block is destroyed, the nodes rendered into the remote element are removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay oklch!

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need / want to inline @glimmer/debug-util so that it's stripped during the build?

@wycats
Copy link
Contributor Author

wycats commented Nov 19, 2024

Do we need / want to inline @glimmer/debug-util so that it's stripped during the build?

Yep, we do that

@NullVoxPopuli NullVoxPopuli merged commit 361b214 into main Nov 19, 2024
5 checks passed
@NullVoxPopuli NullVoxPopuli deleted the feature/error-recovery-redux-pt3 branch November 19, 2024 21:44
@github-actions github-actions bot mentioned this pull request Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants