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

Jets for min, max, and peg #117

Merged
merged 15 commits into from
Nov 15, 2023
Merged

Jets for min, max, and peg #117

merged 15 commits into from
Nov 15, 2023

Conversation

ashelkovnykov
Copy link
Contributor

No description provided.

Base automatically changed from as/mink-scry to status October 26, 2023 05:36
@ashelkovnykov ashelkovnykov marked this pull request as ready for review October 26, 2023 05:54
rust/ares/Cargo.toml Outdated Show resolved Hide resolved
rust/ares/src/jets/bits.rs Outdated Show resolved Hide resolved
rust/ares/src/jets/tree.rs Show resolved Hide resolved
@eamsden
Copy link
Collaborator

eamsden commented Nov 10, 2023

I rewrote jet_peg and jet_mas to use bitslices: 4177ae6

@ashelkovnykov
Copy link
Contributor Author

@eamsden I applied your changes.

There was a "wrong bitshift direction" bug that didn't get caught by the tests, because it would allocate too many words, so there was no actual problem (just memory inefficiency). I added tests for the case, but it looks like the bug is not detectable after the fact because the excessive size is resolved by normalization to atom.

I'll leave the tests in, but I'm not sure how we would catch bugs like this with tests.

@eamsden
Copy link
Collaborator

eamsden commented Nov 15, 2023

@eamsden I applied your changes.

There was a "wrong bitshift direction" bug that didn't get caught by the tests, because it would allocate too many words, so there was no actual problem (just memory inefficiency). I added tests for the case, but it looks like the bug is not detectable after the fact because the excessive size is resolved by normalization to atom.

I'll leave the tests in, but I'm not sure how we would catch bugs like this with tests.

Good catch!

The test would have to be somewhat imprecise and check that the allocation didn't exceed what was reasonable. That wrong bitshift would have increased the allocation size from what was necessary by a factor of 4k, so it shouldn't be too hard to catch with thresholding. I'll make an issue to have tests like this.

@@ -64,16 +63,15 @@ pub fn jet_peg(context: &mut Context, subject: Noun) -> Result {
let b_bits = met(0, b);
let out_bits = a_bits + b_bits - 1;

let out_words = (out_bits + 63) << 6; // bits to 8-byte words
let out_words = (out_bits + 63) >> 6; // bits to 8-byte words
Copy link
Collaborator

Choose a reason for hiding this comment

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

good catch!

@eamsden eamsden merged commit 7a82b5d into status Nov 15, 2023
1 check passed
@eamsden eamsden deleted the sigilante/jets-one branch November 15, 2023 12:38
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.

3 participants