Add SequentialIdentities to avoid RdId collisions#588
Add SequentialIdentities to avoid RdId collisions#588Iliya-usov wants to merge 1 commit intomasterfrom
Conversation
Problem:
The old Identities class mixed parent ID into dynamic IDs, which caused
collisions.
Solution:
- Add SequentialIdentities class with two ID strategies:
* Dynamic IDs (Next): Sequential integers ignoring parent, preventing
order-dependent collisions. Client IDs are even, server IDs are odd.
* Stable IDs (Mix): Hash-based with the high bit set (0x8000000000000000)
ensuring they never collide with dynamic IDs.
- Add Identities to SerializationCtx for consistent ID generation during
serialization (required for intern roots).
- Mark old Identities class as deprecated/obsolete.
- Make RdIdUtil internal to prevent accidental direct usage.
API changes in IIdentities:
- Next(parent): generates next dynamic ID
- Mix(rdId, tail): creates stable ID from hash
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 86 out of 88 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| myInternRoot.RdId = RdId.Mix("InternRoot"); | ||
| myProtocolValueSet.RdId = RdId.Mix("ValueSet"); | ||
| myInternRoot.RdId = proto.Identities.Mix(RdId, "InternRoot");; |
There was a problem hiding this comment.
There's an extra semicolon at the end of this line. It should be a single semicolon, not a double semicolon.
| } | ||
| } | ||
|
|
||
| public static class RdIdUtil |
There was a problem hiding this comment.
According to the PR description, RdIdUtil should be made internal to prevent accidental direct usage. However, this class is still marked as public. Consider changing it to internal.
rd-kt/rd-framework/src/test/kotlin/com/jetbrains/rd/framework/test/cases/RdSignalTest.kt
Show resolved
Hide resolved
| foreach (var x in items) | ||
| { | ||
| (x as IRdBindable).IdentifyEx(ids, id.Mix(i++)); | ||
| (x as IRdBindable).IdentifyEx(ids, ids.Mix(id, i++)); |
There was a problem hiding this comment.
It seems like this thing could also be migrated to seq ids. I'm not sure how to preserve backward compatibility, though. Maybe it could be in another PR.
|
I have a similar thing in ultimate in branch, but would be happy to switch to your impl. Thanks! |
| * Utility object for RdId hash computation operations. | ||
| * This is internal - for ID generation, use [IIdentities.mix] and [IIdentities.next]. | ||
| */ | ||
| object RdIdUtil { |
There was a problem hiding this comment.
(Minor) Let's mark the type itself internal then. No sense in keeping it public with no public methods anyway.
3 participants
Problem:
The old Identities class mixed parent ID into dynamic IDs, which caused collisions.
Solution:
Add SequentialIdentities class with two ID strategies:
Add Identities to SerializationCtx for consistent ID generation during serialization (required for intern roots).
Mark old Identities class as deprecated/obsolete.
Make RdIdUtil internal to prevent accidental direct usage.
API changes in IIdentities: