Skip to content
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

[RFC007] Bytecode interpreter #2045

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

[RFC007] Bytecode interpreter #2045

wants to merge 19 commits into from

Conversation

yannham
Copy link
Member

@yannham yannham commented Sep 17, 2024

Although the name is a bit pompous, the goal of this RFC is mostly to be a working document for designing a more compact and efficient run-time representation for Nickel expressions.

While this is something that won't be user-facing (at least in a direct way), and thus can be changed later without breaking backward-compatibility, I think the technical scope of this effort is such that I find it better to discuss it formally here before going for a first implementation.

@github-actions github-actions bot temporarily deployed to pull request September 17, 2024 15:08 Inactive
Copy link
Contributor

github-actions bot commented Sep 17, 2024

🐰 Bencher Report

Branch2045/merge
Testbedubuntu-latest

⚠️ WARNING: The following Measure does not have a Threshold. Without a Threshold, no Alerts will ever be generated!

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds CLI flag.

Click to view all benchmark results
BenchmarkLatencynanoseconds (ns)
fibonacci 10📈 view plot
⚠️ NO THRESHOLD
488,970.00
foldl arrays 50📈 view plot
⚠️ NO THRESHOLD
1,916,800.00
foldl arrays 500📈 view plot
⚠️ NO THRESHOLD
25,113,000.00
foldr strings 50📈 view plot
⚠️ NO THRESHOLD
7,363,100.00
foldr strings 500📈 view plot
⚠️ NO THRESHOLD
65,138,000.00
generate normal 250📈 view plot
⚠️ NO THRESHOLD
47,324,000.00
generate normal 50📈 view plot
⚠️ NO THRESHOLD
2,059,700.00
generate normal unchecked 1000📈 view plot
⚠️ NO THRESHOLD
59,119,000.00
generate normal unchecked 200📈 view plot
⚠️ NO THRESHOLD
2,911,900.00
pidigits 100📈 view plot
⚠️ NO THRESHOLD
3,252,400.00
pipe normal 20📈 view plot
⚠️ NO THRESHOLD
1,499,800.00
pipe normal 200📈 view plot
⚠️ NO THRESHOLD
12,879,000.00
product 30📈 view plot
⚠️ NO THRESHOLD
843,320.00
scalar 10📈 view plot
⚠️ NO THRESHOLD
1,529,500.00
sum 30📈 view plot
⚠️ NO THRESHOLD
846,870.00
🐰 View full continuous benchmarking report in Bencher

@github-actions github-actions bot temporarily deployed to pull request September 19, 2024 16:26 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 20, 2024 14:58 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 23, 2024 08:32 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 23, 2024 10:47 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 23, 2024 16:39 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 25, 2024 12:38 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 25, 2024 16:07 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 30, 2024 14:33 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 3, 2024 17:27 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 6, 2024 16:52 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 6, 2024 21:38 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 7, 2024 15:22 Inactive
@yannham yannham marked this pull request as ready for review October 15, 2024 13:06
@yannham
Copy link
Member Author

yannham commented Oct 15, 2024

Some parts might need refinement, but I think it's in a good shape for a first round of reviews.

#### AST

The first one is an AST and would more or less correspond to the current unique
representation, minus runtime-specific constructors. We could have gone closer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can also get rid of some efficiency-oriented duplication in the current representation, like the distinction between LetPattern/Let and RecRecord/Record. Getting rid of these would be convenient for both the LSP and the typechecker, I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. In fact I started to draft the first representation and did get rid of the Let and Fun to keep only the pattern variants.

record and the empty array, for example with `enum Record { Empty,
NonEmpty(RecordData) }`. This should use the same space as `RecordData` in Rust
(if `RecordData` is a pointer, at least) and save an allocation for empty
structures.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused about what discriminants are present in the "top-level" representation (I'm not sure what's the right term, but I'm talking about the word-sized thing that packs in a pointer with some discriminant). Above, it sounded like we were only going to inline null and boolean in the discriminant; here, you're also proposing the put empty records and empty arrays? It seems like there isn't enough room in the pointer alignment for all of these.

By the way, x86_64, aarch64, and riscv-64 all max out at 48 bits of address space. So on these architectures we can pack lots more stuff at the most-significant end of the top-level representation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, x86_64, aarch64, and riscv-64 all max out at 48 bits of address space. So on these architectures we can pack lots more stuff at the most-significant end of the top-level representation.

Yes, but it's less portable. Although we don't need Nickel to run on embedded, I think we can do with one bit for now, and explore those other possibilities later.

I'm a bit confused about what discriminants are present in the "top-level" representation (I'm not sure what's the right term, but I'm talking about the word-sized thing that packs in a pointer with some discriminant). Above, it sounded like we were only going to inline null and boolean in the discriminant; here, you're also proposing the put empty records and empty arrays? It seems like there isn't enough room in the pointer alignment for all of these.

Your first understanding is right. At the top-level, there is only one discriminant for bool, null and pointer. If we follow the pointer, we find many representations: arrays, numbers, etc. Here I'm talking about the representation of the pointee, which can itself be a pointer to something else (typically I guess your immutable vec representation would be mostly a pointer to the root plus some parameters). Somehow the 1-word representation can hold any data, and I'm talking about this specific data when the pointee represents a record. Does that make sense?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, as we need a discriminant at the beginning of the pointee (is it an Array? a Record? etc.) that will need to be aligned (although it might be merged with some other metadata as well), we'll probably have some more space here, and can even special case EmptyArray and EmptyRecord as special discriminants, instead of bothering making Record actually an enum.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even on 32-bit, we can have lots more values at the top-level, right? Assuming our pointers are all 4-byte aligned, anything ending in 01, 10, and 11 is not a pointer. But then for each of those non-pointer values we still have 30 bits left to store actual data. So couldn't we have variants for EmptyArray and EmptyRecord without even following that single top-level pointer? And we'd still have room for small integers...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we have a lot more room. It's just that you can't encode anything that needs at least a full machine word or more - typically OCaml needs to have 31-bits and 63-bits integers so that they can unbox them, which is the case of most other non trivial data structures. But indeed special values like empty stuff could in theory be also directly put that top-level.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, makes sense. I was just confused about what discriminants were where. Our current Vector has a root: Option<Rc<Node>>, so it's already avoiding allocation for empty arrays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants