Skip to content

Conversation

wycats
Copy link
Contributor

@wycats wycats commented Jan 23, 2025

No description provided.

Copy link
Contributor

github-actions bot commented Jan 23, 2025

This PRmain
Dev
740K └─┬ .
233K   ├── syntax
171K   ├── runtime
161K   ├── compiler
 72K   ├── opcode-compiler
 27K   ├── manager
 24K   ├── validator
 13K   ├── program
8.9K   ├── reference
7.2K   ├── destroyable
6.4K   ├── util
4.3K   ├── node
3.4K   ├── global-context
2.6K   ├── wire-format
1.0K   ├── vm
969B   ├── encoder
844B   ├── vm-babel-plugins
606B   └── owner
588K └─┬ .
169K   ├── runtime
160K   ├── syntax
100K   ├── compiler
 58K   ├── opcode-compiler
 27K   ├── manager
 24K   ├── validator
 11K   ├── program
8.9K   ├── reference
7.2K   ├── destroyable
6.3K   ├── util
4.3K   ├── node
3.4K   ├── global-context
2.5K   ├── wire-format
1.0K   ├── vm
969B   ├── encoder
844B   ├── vm-babel-plugins
606B   └── owner
Prod
297K └─┬ .
103K   ├── syntax
 72K   ├── compiler
 64K   ├── runtime
 22K   ├── opcode-compiler
7.9K   ├── manager
5.6K   ├── program
5.1K   ├── validator
3.6K   ├── reference
2.5K   ├── util
2.1K   ├── node
1.8K   ├── wire-format
1.5K   ├── destroyable
724B   ├── vm
594B   ├── global-context
516B   ├── encoder
469B   ├── vm-babel-plugins
155B   └── owner
231K └─┬ .
 70K   ├── syntax
 63K   ├── runtime
 48K   ├── compiler
 18K   ├── opcode-compiler
7.9K   ├── manager
5.1K   ├── validator
4.8K   ├── program
3.6K   ├── reference
2.4K   ├── util
2.1K   ├── node
1.6K   ├── wire-format
1.5K   ├── destroyable
737B   ├── vm
594B   ├── global-context
516B   ├── encoder
469B   ├── vm-babel-plugins
155B   └── owner

@wycats wycats force-pushed the feature/emit-fn-calls branch 2 times, most recently from cc230f3 to 4625e5e Compare February 6, 2025 07:26
@wycats wycats force-pushed the feature/emit-fn-calls branch from a5f57d9 to cddc4dc Compare February 12, 2025 02:52
@wycats wycats force-pushed the feature/emit-fn-calls branch from 6a5d569 to 87e01ac Compare February 20, 2025 03:18
NullVoxPopuli added a commit that referenced this pull request Feb 20, 2025
NullVoxPopuli added a commit that referenced this pull request Feb 20, 2025
NullVoxPopuli added a commit that referenced this pull request Feb 20, 2025
NullVoxPopuli added a commit that referenced this pull request Feb 20, 2025
NullVoxPopuli added a commit that referenced this pull request Feb 20, 2025
@github-actions github-actions bot mentioned this pull request Feb 20, 2025
@wycats wycats force-pushed the feature/emit-fn-calls branch 3 times, most recently from 0683762 to 9ba08a7 Compare February 23, 2025 08:37
@github-actions github-actions bot mentioned this pull request Mar 4, 2025
@wycats wycats force-pushed the feature/emit-fn-calls branch 2 times, most recently from 2b97bec to ecadfb7 Compare March 19, 2025 01:04
@NullVoxPopuli NullVoxPopuli force-pushed the feature/emit-fn-calls branch from ecadfb7 to ccb6165 Compare March 25, 2025 13:56
@NullVoxPopuli NullVoxPopuli requested a review from Copilot March 26, 2025 21:11
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR begins transitioning towards an invokable wire format by updating integration test harnesses, refining event recording and cache behavior, and adding comprehensive documentation and pseudocode for the Glimmer reactivity system.

  • Updated integration tests with new CSS and improved event recording handling.
  • Refactored build configuration to adjust the typescript helper and updated the benchmark environment to reference layout compilation.
  • Added extensive documentation and pseudocode for the reactive system, including tags, primitives, and composition.

Reviewed Changes

Copilot reviewed 171 out of 176 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/@glimmer-workspace/integration-tests/lib/setup-harness.ts Added import for new CSS tweaks.
packages/@glimmer-workspace/integration-tests/lib/render-test.ts Extended test interfaces and recording event functionality.
packages/@glimmer-workspace/integration-tests/lib/modes/jit/delegate.ts Minor whitespace adjustment.
packages/@glimmer-workspace/integration-tests/lib/compile.ts Updated precompileJSON call with meta information.
packages/@glimmer-workspace/integration-tests/index.ts Updated exports to include additional test helpers.
packages/@glimmer-workspace/build/lib/config.js Refactored typescript function signature and adjusted SWC settings.
packages/@glimmer-workspace/benchmark-env/lib/benchmark/util.ts Replaced reference to compilable with layout in entry compilation.
guides/reactivity/* New and updated documentation and pseudocode for the reactivity system.
Files not reviewed (5)
  • .prototools: Language not supported
  • .vscode/settings.json: Language not supported
  • package.json: Language not supported
  • packages/@glimmer-workspace/build/package.json: Language not supported
  • packages/@glimmer-workspace/integration-tests/lib/harness/tweaks.css: Language not supported
Comments suppressed due to low confidence (3)

packages/@glimmer-workspace/build/lib/config.js:410

  • The typescript() function signature was changed by removing the package parameter. Please ensure all callers have been updated accordingly and update any related documentation or JSDoc to reflect this change.
              typescript(env),

packages/@glimmer-workspace/benchmark-env/lib/benchmark/util.ts:5

  • The property reference was changed from 'compilable' to 'layout'. Verify that 'layout' exists on entry and that this change is consistent with the intended API to avoid compilation issues.
  return unwrapHandle(entry.layout!.compile(context));

packages/@glimmer-workspace/integration-tests/lib/render-test.ts:112

  • [nitpick] This diff introduces private class fields (using the '#' syntax). Ensure that the target TypeScript/JavaScript environment fully supports private fields to avoid runtime errors.
  #events: RecordedEvent[] = [];

@reteps
Copy link

reteps commented Apr 10, 2025

Any way that I can contribute? In the context of #1666, this would be very helpful.

@NullVoxPopuli
Copy link
Contributor

idk, basically what's left here is that I need to add tests, and make sure CI goes green

NullVoxPopuli added a commit to NullVoxPopuli/ember.js that referenced this pull request Apr 25, 2025
NullVoxPopuli added a commit to NullVoxPopuli/ember.js that referenced this pull request Apr 25, 2025
NullVoxPopuli added a commit that referenced this pull request May 28, 2025
NullVoxPopuli added a commit that referenced this pull request May 28, 2025
@github-actions github-actions bot mentioned this pull request Apr 29, 2025
@wycats wycats force-pushed the feature/emit-fn-calls branch 2 times, most recently from 6aa74e5 to dd2e3dd Compare May 29, 2025 06:17
@wycats wycats force-pushed the feature/emit-fn-calls branch from f993a1d to 1edd80c Compare June 30, 2025 21:07
@NullVoxPopuli NullVoxPopuli force-pushed the feature/emit-fn-calls branch from 348faea to db14bd1 Compare July 1, 2025 15:41
@wycats wycats force-pushed the feature/emit-fn-calls branch from db14bd1 to bbd6880 Compare July 7, 2025 03:07
@vlascik
Copy link

vlascik commented Jul 8, 2025

So I've been watching this PR in a hope ember will finally get its render performance act together, but I can't help myself but think this rewrite is going to miss the mark, again.

In my opinion, when it comes to the JS performance, two things are true:

  1. You cannot get faster than vanillajs. vanillajs itself might get faster, but that's the JS engine authors' job, not the framework's.

  2. The only way to be as fast as vanillajs, is to BE vanillajs.

I hope I'm wrong, but this PR doesn't seem to be headed into that direction (yet?).

If you profile say the simplest template, with just one <div>, flamegraphs show glimmer spends more time in JS execution of the VM than in the browser's createElement call - and that is what is causing the ember's 2x slowdown compared to every other framework. The code, that should have been just a simple createElement call, seems to be running up and down the VM stack, popping and allocating and doing JS work that it shouldn't be doing at all. The VM should only unpack the wire format once upon the first template use, the result should be something along the lines of templateRenderFunctionCache['my-template'] = new Function("document.createElement('div')") and from then on, the only thing executed should be the templateRenderFunctionCache['my-template']() and nothing else - no VM, no extra JS execution, no overhead to pay except for the first conversion from the wire format to vanilla JS.

Otherwise, you might shave off few milliseconds here and there, but this will never get fast enough to compete with frameworks executing vanillajs code or vanillajs itself.

I really hope I'm wrong and that is actually the plan, but upon (brief, I admit, don't have time to dig into this) look at this PR, it doesn't seem to be headed that way.

Thoughts?

@wycats wycats force-pushed the feature/emit-fn-calls branch from a4959bf to 493d360 Compare July 14, 2025 21:03
@wycats wycats force-pushed the feature/emit-fn-calls branch 2 times, most recently from ffeab81 to 9f5a7d1 Compare July 21, 2025 23:50
@wycats
Copy link
Contributor Author

wycats commented Jul 21, 2025

I hope I'm wrong, but this PR doesn't seem to be headed into that direction (yet?).

@vlascik "(yet?)" is right. I'm trying to do this a little at a time, and even this incremental piece has been a somewhat significant project.

That said, the goal here is to get to a point where, first, the wire format itself is not needed. Next, we'd migrate more of the opcode compiler to vanilla-js (although I think you might be underestimating what's needed to make "vanilla js" support updating). If we can get to the end of this PR, I'll feel pretty optimistic about the direction you're suggesting.

And apologies for how long it's taken!

@wycats wycats force-pushed the feature/emit-fn-calls branch 4 times, most recently from 4e47e99 to bb14bb0 Compare July 22, 2025 23:47
This commit introduces major improvements to the expression handling and opcode compilation:

Expression Type Refactoring:
- Flattened Expression type to only include StackExpression, GetVar, and GetKeyword
- Removed nested expression types in favor of linear stack operations
- Fixed type errors throughout the codebase from the flattening

GetKeyword Support:
- Added proper GetKeyword type to Expression union
- Implemented GetKeyword formatting in wire-format-debug.ts
- Fixed wire format debug formatting for stack operations

Stack Operation Improvements:
- Converted all expressions to use linear stack-based operations
- Removed $v0 special register usage
- Simplified expression compilation with direct stack manipulation

Code Quality:
- Fixed formatCallArgs typo (should be formatArgs)
- Improved type safety with proper SimpleStackOp handling
- Enhanced debug formatting for better developer experience

These changes improve compile-time type checking and runtime performance by
eliminating nested expression evaluation in favor of direct stack operations.
@wycats wycats force-pushed the feature/emit-fn-calls branch from bb14bb0 to d23d59d Compare July 22, 2025 23:54
wycats and others added 9 commits July 22, 2025 17:29
- Add missing Chrome flags needed for headless operation in CI:
  --disable-gpu, --disable-dev-shm-usage, --disable-software-rasterizer
- Add 30-second timeout for Vite startup to prevent hanging
- These changes should fix the Chrome and Floating Dependencies tests
  that were hanging in CI
- Strip ANSI codes from Vite output before regex matching
- Check both stdout and stderr for Vite server URL
- Add debug logging to diagnose CI hanging issues
- Import strip-ansi to handle colored terminal output

The CI tests were hanging because Vite's colored output with ANSI
escape sequences wasn't being matched by the port detection regex.
- Add buffering for both stdout and stderr streams
- Accumulate output before checking for port regex match
- Add more detailed debug logging to track buffer state
- Handle https in addition to http URLs

The CI was likely failing because Vite's output was arriving in
multiple chunks, and the port URL was split across chunk boundaries.
- Add portFound flag to track when port detection succeeds
- Return early from data handlers to avoid unnecessary processing
- Prevent buffer accumulation after port is detected
- Keep stderr output for logging but skip regex matching

This optimization prevents wasting CPU cycles and memory on
processing Vite output after we've found what we need.
- Applied prettier formatting to bin/run-tests.mjs
- No functional changes, only code style adjustments
- Replace @pnpm/workspace.find-packages with repo-metadata solution
- Add Playwright-based browser automation utilities
- Refactor benchmark tools (bench-quick, setup-bench) to use shared abstractions
- Implement proper process cleanup with tree-kill
- Add benchmark completion signaling via benchmarkComplete()
- Fix TypeScript issues and improve type safety
- Reorganize bin/ directory structure for better organization
- Add recording capabilities for screenshots and videos
- Remove obsolete patch scripts

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Document current state and issues with setup-bench.mts
- Propose improvements to align with bench-quick.mts patterns
- Outline phased implementation approach
- Define benefits and considerations for the refactoring

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants