perf,fix(zero-solid): back to produce but pre-reading relationships and optimizing initial view build#3701
Merged
perf,fix(zero-solid): back to produce but pre-reading relationships and optimizing initial view build#3701
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
grgbkr
force-pushed
the
grgbkr/mlaw-solid
branch
from
525b32b to
61eaf97
Compare
|
Bencher Report| Branch | grgbkr/mlaw-solid |
| 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 | 930.59 (+0.13%) | 947.93 (98.17%) |
| zero.js | 📈 view plot 🚷 view threshold | 175.26 (0.00%) | 178.77 (98.04%) |
| zero.js.br | 📈 view plot 🚷 view threshold | 48.82 (-0.06%) | 49.83 (97.98%) |
|
| Branch | grgbkr/mlaw-solid |
| 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 | 73.50 (+0.55%) | 68.31 (92.94%) |
| src/client/zero.bench.ts > with filter > Lower rows 500 x 10 columns (numbers) | 📈 view plot 🚷 view threshold | 90.61 (-4.35%) | 90.28 (99.63%) |
grgbkr
commented
Feb 4, 2025
| * `EditChange#node` and `EditChange#oldNode`. The `ViewChange` type | ||
| * documents and enforces this via the type system. | ||
| */ | ||
| export type ViewChange = |
Contributor
Author
There was a problem hiding this comment.
I don't love having these new types exposed in @rocicorp/zero/advanced, but it seemed better than just dropping the relationships from ChildChange and EditChange.
arv
reviewed
Feb 4, 2025
| function materializeNodesRelationships(node: Node): Node { | ||
| return { | ||
| row: node.row, | ||
| relationships: Object.fromEntries( |
Contributor
There was a problem hiding this comment.
Do we really need all these temporary arrays?
I'm worried about the strain we are putting on the GC here
arv
reviewed
Feb 4, 2025
Contributor
arv
left a comment
arv
left a comment
There was a problem hiding this comment.
LGTM
I still don't feel good about this. Why is it so slow?
Contributor
|
Remember that the test case is pushing like 9k rows. So it's 1.5ms/row.
Which ... still surprisingly slow, but tolerable.
…On Mon, Feb 3, 2025 at 10:24 PM Erik Arvidsson ***@***.***> wrote:
***@***.**** commented on this pull request.
LGTM
I still don't feel good about this. Why is it so slow?
—
Reply to this email directly, view it on GitHub
<#3701 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAATUBHLB2AYCWTUBDYJT4T2OB2NDAVCNFSM6AAAAABWNR7M5CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKOJSGA3TAMBRGQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Contributor
|
Sorry I mean 0.01ms/row. Maybe within range of reason.
On Mon, Feb 3, 2025 at 11:31 PM Aaron Boodman ***@***.***>
wrote:
… Remember that the test case is pushing like 9k rows. So it's 1.5ms/row.
Which ... still surprisingly slow, but tolerable.
On Mon, Feb 3, 2025 at 10:24 PM Erik Arvidsson ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
>
> LGTM
>
> I still don't feel good about this. Why is it so slow?
>
> —
> Reply to this email directly, view it on GitHub
> <#3701 (review)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAATUBHLB2AYCWTUBDYJT4T2OB2NDAVCNFSM6AAAAABWNR7M5CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKOJSGA3TAMBRGQ>
> .
> You are receiving this because you are subscribed to this thread.Message
> ID: ***@***.***>
>
|
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
4 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.
The
reconcileoptimization 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 byapplyChanges.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.
This is because the proxy used by solid
produceis 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 solidproduce. This brings the time for the 3000 row initial load case in hello-zero-solid to ~133ms.