-
Notifications
You must be signed in to change notification settings - Fork 594
Make NodeId and SymbolId into uint64s, fix up KeyBuilder #243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
internal/checker/checker.go
Outdated
@@ -11211,19 +11211,31 @@ var base64chars = []byte{ | |||
'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', '$', '%', | |||
} | |||
|
|||
func (b *KeyBuilder) WriteInt(value int) { | |||
func (b *KeyBuilder) WriteUint64(value uint64) { | |||
for value != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is subtly wrong thanks to sign extension, so when we cast to int
outside it's possible we break. But we wouldn't have observed it yet because we haven't gotten up to MaxInt in any test so far (but would in a build mode, editor, etc)
Hmm, I don't know about this. Each symbol consumes 120 bytes just for the struct itself, plus what it references. That's at least 500Gb of allocated symbols for which we have to request IDs before an overflow occurs (but more likely multiple terabytes). Nodes are similar. In fact, the checker rarely asks for node IDs. I really don't think we need this. I'd sooner expand |
For batch compilation, I generally agree, but for long-running processes like tsserver or the API, I don't think it's entirely impossible for it to get to this point after a lot of typing and no restarting. That's the main concern I'm having. (That's why I'm not concerned about checkers, since those are tossed on every change) |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Files not reviewed (1)
- internal/ast/ids.go: Evaluated as low risk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Files not reviewed (1)
- internal/checker/checker.go: Evaluated as low risk
In looking at these structs, I realized that these IDs can be
uint64
s without affecting any sizing at all. This would make me a lot less afraid of us overflowing on these shared resources.The only other places these IDs would be used in map keys, but even if a map contained a million entries, the amount of memory used for the keys would only increase by 4MB. And, we're honestly more likely to use node pointers as keys anyhow.
I did not touch type IDs, which are local to a checker, and I think less likely to overflow because of that.