webxdc/pixel
16
4
Fork
You've already forked pixel
1

detect and handle concurrency using a logical clock #6

Merged
hpk merged 2 commits from ansuz/pixel:convergence into main 2024年02月11日 18:02:41 +01:00
Contributor
Copy link

This patch adds lamport timestamps to updates, allowing remote peers to determine whether peers were aware of each others' state at the time when they made a change.

Assuming there's no flaws in my logic, it should guarantee that peers arrive at the same eventual state regardless of the order in which updates are received or applied (addressing issue #5).

This patch adds lamport timestamps to updates, allowing remote peers to determine whether peers were aware of each others' state at the time when they made a change. Assuming there's no flaws in my logic, it should guarantee that peers arrive at the same eventual state regardless of the order in which updates are received or applied (addressing [issue #5](https://codeberg.org/webxdc/pixel/issues/5)).
WofWca left a comment
Copy link

From #5

@hpk and I discussed referencing this app as a minimal example of how to accomplish this without needing to pull in a big library like Yjs

In a practical sense, I think this is an huge overkill for such an app, so perhaps it makes sense to also reflect this fact in README, so that potential webxdc developers don't go

Oh, so in order to write a webxdc app I need to read this huge math paper and implement a CRDT LWW LGBT algorithm.
Thanks, I'll pass.

From #5 > @hpk and I discussed referencing this app as a minimal example of how to accomplish this without needing to pull in a big library like Yjs In a practical sense, I think this is an huge overkill for such an app, so perhaps it makes sense to also reflect this fact in README, so that potential webxdc developers don't go > Oh, so in order to write a webxdc app I need to read this huge math paper and implement a CRDT LWW LGBT algorithm. > Thanks, I'll pass.
@ -7,0 +14,4 @@
// Uint16 gives us up to 65535 updates, which is probably enough
// this can always be converted to a Uint32Array during runtime
// or even an array of BigInts if we really want to prove 'correctness'
let recency = new Uint16Array(size);
Owner
Copy link

I would just use Uint32Array. Not sure what "this can always be converted to a Uint32Array during runtime" means, does it happen automatically?

I would just use Uint32Array. Not sure what "this can always be converted to a Uint32Array during runtime" means, does it happen automatically?
Author
Contributor
Copy link

I thought that might have been a bit unclear. It won't happen automatically, but it's relatively little code to implement it.

Putting this at the top of the setPixel function which handles updating the local structures would make it so that the smaller structure would be used until the larger one was required.

if (sequence > 65535 && sequence instanceof Uint16Array) {
 // upgrade to a larger TypedArray
 recency = new Uint32Array(recency);
}

I think it will probably be very rare in practice for any given group to send more than 2 ** 16 updates in this app. From the perspective of using it as an example, though, it can help illustrate that algorithms for distributed state can fail under trivial circumstances like overflows.

Aside from that, I figured that showing how to use less memory would make the app a better starting point for forks, like if anyone wanted to drastically increaes the grid size.

Maybe this PR isn't the right avenue for such a change, as it's mostly secondary to the issue of handling concurrency. An array would almost certainly be fast and small enough for the current grid size, and it's very unlikely that anyone would overflow it with 2 ** 53 updates.

I thought that might have been a bit unclear. It won't happen automatically, but it's relatively little code to implement it. Putting this at the top of the `setPixel` function which handles updating the local structures would make it so that the smaller structure would be used until the larger one was required. ```js if (sequence > 65535 && sequence instanceof Uint16Array) { // upgrade to a larger TypedArray recency = new Uint32Array(recency); } ``` I think it will probably be very rare in practice for any given group to send more than `2 ** 16` updates in this app. From the perspective of using it as an example, though, it can help illustrate that algorithms for distributed state can fail under trivial circumstances like overflows. Aside from that, I figured that showing how to use less memory would make the app a better starting point for forks, like if anyone wanted to drastically increaes the grid size. Maybe this PR isn't the right avenue for such a change, as it's mostly secondary to the issue of handling concurrency. An array would almost certainly be fast and small enough for the current grid size, and it's very unlikely that anyone would overflow it with `2 ** 53` updates.
ansuz marked this conversation as resolved

Why is it still a WIP?

Why is it still a WIP?
Author
Contributor
Copy link

Why is it still a WIP?

At first I just wanted to indicate that I wasn't expecting it to get merged as-is so there wouldn't be any pressure.

Assuming there's no flaws in my logic...

Since then I noticed a possible flaw in the logic 😆

the docs say that:

All peers, including the sending one, will receive the update by the callback given to setUpdateListener()

...but that means that there's an extra asynchronous step in between the mouseDownHandler and our own update's information being set in the local data structures. That makes it a little harder to reason about and prove that it will behave as expected.

I'm not certain there could be a race condition in practice, but calling setPixel from within that handler makes everything more predictable, especially since there is a draw() call which will read from the (not yet updated) pixels array. The downside is that you'd end up processing your own update twice, when sending it and when receiving it, though it will effectively be ignored the second time around.

I've pushed a new convergence-amendments branch which integrates changes for both this issue andd converting the recency array to a larger format. The diff between this PR and that branch is here.

> Why is it still a WIP? At first I just wanted to indicate that I wasn't expecting it to get merged as-is so there wouldn't be any pressure. > Assuming there's no flaws in my logic... Since then I noticed a possible flaw in the logic 😆 [the docs](https://docs.webxdc.org/spec/sendUpdate.html) say that: > All peers, including the sending one, will receive the update by the callback given to setUpdateListener() ...but that means that there's an extra asynchronous step in between the `mouseDownHandler` and our own update's information being set in the local data structures. That makes it a little harder to reason about and prove that it will behave as expected. I'm not certain there could be a race condition in practice, but calling `setPixel` from within that handler makes everything more predictable, especially since there is a `draw()` call which will read from the (not yet updated) `pixels` array. The downside is that you'd end up processing your own update twice, when sending it and when receiving it, though it will effectively be ignored the second time around. I've pushed a new `convergence-amendments` branch which integrates changes for both this issue andd converting the `recency` array to a larger format. The diff between this PR and that branch is [here](https://codeberg.org/ansuz/pixel/compare/convergence...ansuz/pixel:convergence-amendments).
hpk approved these changes 2024年01月31日 12:14:00 +01:00
hpk left a comment
Copy link

nice PR. Maybe rename "sequence" to "lamport_timestamp" or "seqnum" or so? "sequence" conveys a list type to me :)

nice PR. Maybe rename "sequence" to "lamport_timestamp" or "seqnum" or so? "sequence" conveys a list type to me :)
Author
Contributor
Copy link

nice PR. Maybe rename "sequence" to "lamport_timestamp" or "seqnum" or so? "sequence" conveys a list type to me :)

That makes sense, though I think renaming now might cause conflicts with the other branch, so I'd prefer to wait until there's a consensus on those changes before adding more.

> nice PR. Maybe rename "sequence" to "lamport_timestamp" or "seqnum" or so? "sequence" conveys a list type to me :) That makes sense, though I think renaming now might cause conflicts with [the other branch](https://codeberg.org/ansuz/pixel/compare/convergence...ansuz/pixel:convergence-amendments), so I'd prefer to wait until there's a consensus on those changes before adding more.
* use Uint32Array instead of Uint16
* use clearer variable names
* factor some duplicated code into functions
* basic validation for updates so correctness of Lamport timestamps is more obvious
ansuz changed title from (削除) WIP: detect and handle concurrency using a logical clock (削除ここまで) to detect and handle concurrency using a logical clock 2024年02月05日 06:10:34 +01:00
Author
Contributor
Copy link

My latest commit updates some variable names and comments to clarify how the sequence numbers are supposed to work.

I opted against implementing my earlier proposal to dynamically update the type used for sequence numbers for the sake of simplicity. A malicious user could cause problems by sending an update with the maximum valid sequence number, but that's generally outside the threat model of most webxdc apps.

My latest commit updates some variable names and comments to clarify how the sequence numbers are supposed to work. I opted against implementing my earlier proposal to dynamically update the type used for sequence numbers for the sake of simplicity. A malicious user could cause problems by sending an update with the maximum valid sequence number, but that's generally outside the threat model of most webxdc apps.
Author
Contributor
Copy link

Oh, and I removed the WIP status from the PR. I think it's ok to merge now.

Oh, and I removed the `WIP` status from the PR. I think it's ok to merge now.
hpk approved these changes 2024年02月08日 14:45:09 +01:00
hpk left a comment
Copy link

Still looks fine to merge. Some minor suggestions to improve readability. Generally good to introduce as few "terms" into the variable namings as possible, for a refined example.

Still looks fine to merge. Some minor suggestions to improve readability. Generally good to introduce as few "terms" into the variable namings as possible, for a refined example.
@ -7,0 +21,4 @@
// this will be updated to the greatest Lamport timestamp
// which we have observed,
let mostRecentLogicalTime = 0;
Owner
Copy link

i'd use mostRecentLamportTime to not introduce another term "Logical" which was not mentioned in the doc-strings.

i'd use `mostRecentLamportTime` to not introduce another term "Logical" which was not mentioned in the doc-strings.
@ -52,2 +70,4 @@
}
function setPixel (offset, value, seqnum) {
// update recency:
Owner
Copy link

such comments obfuscate the reading and don't help much.

such comments obfuscate the reading and don't help much.
@ -54,0 +72,4 @@
function setPixel (offset, value, seqnum) {
// update recency:
recency[offset] = seqnum;
// and update the value
Owner
Copy link

this comment can be removed as well IMO.

this comment can be removed as well IMO.
@ -54,0 +80,4 @@
return y * gridHeight + x;
}
function isPositiveInteger (n) {
Owner
Copy link

isPositiveInterger is called only once -- would be nice to keep number of functions minimal and inline it.

`isPositiveInterger` is called only once -- would be nice to keep number of functions minimal and inline it.
@ -59,0 +90,4 @@
x,
y,
seqnum,
enabled,
Owner
Copy link

as setPixel uses (offset, enabled, seqnum) as parameters, it may make sense to use the same logical order here and swap seqnum and enabled.

as `setPixel` uses (offset, enabled, seqnum) as parameters, it may make sense to use the same logical order here and swap `seqnum` and `enabled`.
Sign in to join this conversation.
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
webxdc/pixel!6
Reference in a new issue
webxdc/pixel
No description provided.
Delete branch "ansuz/pixel:convergence"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?