Conversation
…)" This reverts commit 5a0f847.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Bencher Report| Branch | grgbkr/rollback-solid-perf |
| Testbed | localhost |
Click to view all benchmark results
| Benchmark | File Size | Benchmark Result kilobytes (KB) (Result Δ%) | Upper Boundary kilobytes (KB) (Limit %) |
|---|---|---|---|
| zero-package.tgz | 📈 view plot 🚷 view threshold | 919.20 (+0.00%) | 937.57 (98.04%) |
| zero.js | 📈 view plot 🚷 view threshold | 171.95 (0.00%) | 175.39 (98.04%) |
| zero.js.br | 📈 view plot 🚷 view threshold | 47.96 (0.00%) | 48.92 (98.04%) |
|
| Branch | grgbkr/rollback-solid-perf |
| Testbed | localhost |
Click to view all benchmark results
| Benchmark | Throughput | Benchmark Result operations / second (ops/s) (Result Δ%) | Lower Boundary operations / second (ops/s) (Limit %) |
|---|---|---|---|
| src/client/zero.bench.ts > basics > All 1000 rows x 10 columns (numbers) | 📈 view plot 🚷 view threshold | 76.05 (+4.96%) | 67.96 (89.36%) |
| src/client/zero.bench.ts > with filter > Lower rows 500 x 10 columns (numbers) | 📈 view plot 🚷 view threshold | 95.78 (+1.11%) | 90.32 (94.30%) |
Contributor
|
I have a branch that treats the data as immutable and does the bare updates. My initial goal was to use that for react because in react we currently deep copy (since the react external store needs a new object as a signal that something changed).. If we are using reconcile we should use that. I'll upload my branch for your looking... |
arv
reviewed
Feb 3, 2025
Contributor
arv
left a comment
arv
left a comment
There was a problem hiding this comment.
LGTM but I'm afraid this might have other problems.
| push(change: Change): void { | ||
| // Delay setting the state until the transaction commit. | ||
| this.#pendingChanges.push(change); | ||
| applyChange(this.#root, change, this.#input.getSchema(), '', this.#format); |
Contributor
There was a problem hiding this comment.
Is this correct? We are now changing the root without triggering updates. I'm concerned that there might be some way to see this intermediary state?
Maybe we should have a #newRoot and swap the root to that using reconcile in commit?
Contributor
grgbkr
added a commit
that referenced
this pull request
Feb 4, 2025
…nd optimizing initial view build (#3701) The `reconcile` optimization approach (#3682) was incorrect (I misunderstood how reconcile works), as was revealed by the tests added here: #3692. This approach goes back to using `produce`, but addresses the correctness issue by using an idea @tantaman came up with of reading the relationships of the changes at the time the push is received, but still queueing them to be applied to the solid store state on transaction completion. We are careful to only read relationships in the changes actually used by `applyChanges`. This solved the correctness problem, but while much faster than the 30 seconds prior to us trying to optimize this, the 3000 row initial load case in hello-zero-solid was still spending ~742ms in SolidView#applyChanges. <img width="1434" alt="Pasted Graphic 2" src="https://github.com/user-attachments/assets/1179b9c4-90eb-4e3d-b0c7-e027ec51fb34" /> This is because the proxy used by solid `produce` is fairly slow. To address this, I added an optimization for the case where the store's view is currently empty. If the store is currently empty build up the view on a new plain old JS object root, which is returned for the new state. This avoids building up large views from scratch using solid `produce`. This brings the time for the 3000 row initial load case in hello-zero-solid to ~133ms. <img width="1441" alt="Pasted Graphic 1" src="https://github.com/user-attachments/assets/0297b057-c826-420c-bdab-66ff2ee1f06d" />
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Setting solid store state many times is very slow, even if it is within a Solid batch call. In the hello-zero-solid app, if there are 3000 rows, processing the pushes during initial load takes 30 seconds. Nearly all of the time is in an internal solid function called unwrap.
The previous approach to solving this performance issue, #3608, is incorrect cause we cannot defer processing pushes, pushes have to be processed as received, because the relationship streams will be incorrect if processing is deferred. This was surfaced by issue https://bugs.rocicorp.dev/issue/3488
For comparison here is the performance of that approach:
This new approach maintains a copy of the view as a plain JS object which is updated on each push as the push is received, and then uses solid's
reconcileto update the store at the end of the transaction.The downside is we are ~2xing memory (actually this depends on how much of the data is interned string, its probably actually much less than 2x), and we are diffing the views (which doesnt seem to match the solid way of precise reactivity, we are translating a stream of precise updates into a big diff).
The upside is its