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

SmallAtom optimization #372

Merged
merged 12 commits into from
Feb 9, 2024
Merged

SmallAtom optimization #372

merged 12 commits into from
Feb 9, 2024

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Feb 2, 2024

These commits are best reviewed one at a time.

purpose

This patch introduces a more compact representation of small atoms. A NodePtr is 32 bits, 6 bits identifies the kind of node it points to (Atom or Pair), 26 bits is an index into the vector of atoms or pairs.

This patch adds a 3rd kind of node, SmallAtom, where the value of the atom is stored in the low 26 bits directly. The benefits are:

  1. It saves memory, because we don't need to allocate the atom on the heap, nor the atom itself pointing into the heap
  2. indirections, since we have the value immediately, saving at least two memory lookups that are likely to be cache misses (at least on small computers with a small data cache)

A very large number of atoms are small. In a typical CLVM program for instance, all op-codes and environment paths are small.

format

SmallAtom only support atoms that are:

  1. integers in the canonical representation
  2. positive integers
  3. fit in 26 bits

The 26 bits are interpreted as an unsigned integer.

leading zeros

Since our canonical integer representation requires leading zero bytes for certain positive integers (where the most significant bit in the following byte is set), the leading zero is implied by SmallAtom nodes. The function len_for_value() in allocator.rs implements this behavior. It returns the length an atom would have had, if it has been allocated on the heap.

Allocator::nil(), Allocator::one()

The Allocator always allocate two nodes on construction, nil and one. With this patch, we no longer need to allocate these in the atom list nor on the heap. We can just construct NodePtr representing those values directly, as SmallAtom. In order to still enforce the same limit on the number of atoms that are allowed to be allocated by a program, we subtract 2 from the constant, MAX_NUM_ATOMS.

MAX_NUM_ATOMS

We have a (consensus critical) rule on how many atoms are allowed to be allocated by a program. Since SmallAtoms aren't allocated in the atom_vec list, we need to keep a count of them separately, to ensure the limit is identical as prior to this optimization. We keep a counter small_atoms counting the number of small atoms we've allocated. Whenever we check the MAX_NUM_ATOMS limit, we include this counter.

atom() -> &[u8]

A major challenge with this optimization is in the Allocator's interface. It lets you ask for a pointer to the buffer of any atom. Small atoms don't have a buffer since they're not allocated on the heap. In order to stick to the current interface of the allocator, this function is preserved, using a pool of small buffers to return a pointer to.

We have a ring buffer, temp_vec, with a cursor to the next free slow, temp_idx. When the atom() is requested for a node that is a SmallAtom, we grab the next available slot in the ring buffer, increment the cursor, print the integer into the buffer and returns a reference to it.

This relies on the return values from atom() being short lived, and that we never, at one point, keep more than 64 of these references around.

visit_node()

In order to optimize functions using integers, there's a new function on Allocator called visit_node(), which calls a callback with the value of the node pass into it. This means we can avoid conversions from small integers to BigInt for example.

benchmarks

The results of the benchmarks in this repository follow. This is the output from critcmp:

RPi5

group                                               after                                  before
-----                                               -----                                  ------
deserialize/node_from_bytes                         1.01    539.1±0.62µs        ? ?/sec    1.00    531.6±0.31µs        ? ?/sec
deserialize/node_from_bytes_backrefs                1.03    573.1±0.56µs        ? ?/sec    1.00    555.4±2.25µs        ? ?/sec
deserialize/serialized_length_from_bytes            1.00    352.8±0.17µs        ? ?/sec    1.00    353.6±0.14µs        ? ?/sec
deserialize/serialized_length_from_bytes_trusted    1.02    122.0±0.81µs        ? ?/sec    1.00    119.8±1.43µs        ? ?/sec
run_program/block-2000                              1.00    358.6±0.89ns        ? ?/sec    1.02    364.6±0.21ns        ? ?/sec
run_program/compressed-2000                         1.00   1489.4±1.28ms        ? ?/sec    1.15   1705.7±1.69ms        ? ?/sec
run_program/concat                                  1.01   1027.2±1.84ms        ? ?/sec    1.00  1016.8±11.31ms        ? ?/sec
run_program/count-even                              1.00     22.7±0.03ms        ? ?/sec    1.46     33.2±0.08ms        ? ?/sec
run_program/factorial                               1.00    470.3±2.79ms        ? ?/sec    1.02    478.3±3.81ms        ? ?/sec
run_program/hash-string                             1.00   1127.9±1.38ns        ? ?/sec    1.02   1153.7±0.54ns        ? ?/sec
run_program/hash-tree                               1.00     83.0±0.08ms        ? ?/sec    1.11     92.3±0.11ms        ? ?/sec
run_program/large-block                             1.00    444.1±0.79ms        ? ?/sec    1.10    488.6±0.31ms        ? ?/sec
run_program/loop_add                                1.00       5.6±0.01s        ? ?/sec    1.34       7.5±0.02s        ? ?/sec
run_program/loop_ior                                1.00       5.1±0.00s        ? ?/sec    1.50       7.6±0.03s        ? ?/sec
run_program/loop_not                                1.00       5.3±0.01s        ? ?/sec    1.28       6.8±0.04s        ? ?/sec
run_program/loop_sub                                1.00       5.6±0.01s        ? ?/sec    1.39       7.8±0.01s        ? ?/sec
run_program/loop_xor                                1.00       5.7±0.00s        ? ?/sec    1.30       7.4±0.01s        ? ?/sec
run_program/matrix-multiply                         1.00    382.4±0.24ms        ? ?/sec    1.13    431.3±0.37ms        ? ?/sec
run_program/point-pow                               1.00   1296.1±0.24ms        ? ?/sec    1.00   1297.3±0.33ms        ? ?/sec
run_program/pubkey-tree                             1.00    682.7±0.07ms        ? ?/sec    1.00    683.2±0.10ms        ? ?/sec
run_program/shift-left                              1.00       4.4±0.01s        ? ?/sec    1.01       4.5±0.01s        ? ?/sec
run_program/substr                                  1.00     11.8±0.02ms        ? ?/sec    1.40     16.5±0.02ms        ? ?/sec
run_program/substr-tree                             1.00    254.1±0.19ms        ? ?/sec    1.40    354.8±0.29ms        ? ?/sec
run_program/sum-tree                                1.00    604.8±1.22ms        ? ?/sec    1.11    671.6±0.72ms        ? ?/sec

Perhaps the more interesting benchmarks are the run_generator stress tests in the chia_rs repository.
I ran it on Mac OS (M1) and a RaspberryPi 5 machine.

My main concern was to make the worst case more palatable. Among our stress tests, that's deep-recursion-plus and duplicate-coin-announce.

RPi5

group                                                                after                                  before
-----                                                                -----                                  ------
generator/run_block_generator block-1ee588dc                         1.00    281.4±0.29ms        ? ?/sec    1.11    311.2±0.26ms        ? ?/sec
generator/run_block_generator block-1ee588dc-compressed              1.00    278.8±0.31ms        ? ?/sec    1.10    307.1±0.52ms        ? ?/sec
generator/run_block_generator block-225758                           1.00      7.8±0.02ms        ? ?/sec    1.84     14.4±0.03ms        ? ?/sec
generator/run_block_generator block-225758-compressed                1.00      7.8±0.02ms        ? ?/sec    1.84     14.4±0.02ms        ? ?/sec
generator/run_block_generator block-4671894                          1.00    442.0±0.91ms        ? ?/sec    1.85    818.1±1.37ms        ? ?/sec
generator/run_block_generator block-4671894-compressed               1.00    443.5±1.34ms        ? ?/sec    1.84    816.4±0.63ms        ? ?/sec
generator/run_block_generator block-6fe59b24                         1.00    310.9±0.25ms        ? ?/sec    1.10    342.7±0.27ms        ? ?/sec
generator/run_block_generator block-6fe59b24-compressed              1.00    304.9±0.58ms        ? ?/sec    1.11    338.3±0.29ms        ? ?/sec
generator/run_block_generator block-834752                           1.00     22.5±0.03ms        ? ?/sec    1.11     25.0±0.04ms        ? ?/sec
generator/run_block_generator block-834752-compressed                1.00     22.2±0.02ms        ? ?/sec    1.11     24.8±0.05ms        ? ?/sec
generator/run_block_generator block-b45268ac                         1.00    274.2±0.22ms        ? ?/sec    1.11    305.7±0.11ms        ? ?/sec
generator/run_block_generator block-b45268ac-compressed              1.00    272.9±0.23ms        ? ?/sec    1.11    301.6±0.20ms        ? ?/sec
generator/run_block_generator block-c2a8df0d                         1.00    326.4±0.28ms        ? ?/sec    1.11    361.3±0.17ms        ? ?/sec
generator/run_block_generator block-c2a8df0d-compressed              1.00    323.5±0.21ms        ? ?/sec    1.10    356.3±0.11ms        ? ?/sec
generator/run_block_generator block-e5002df2                         1.00    292.8±0.25ms        ? ?/sec    1.11    324.0±0.16ms        ? ?/sec
generator/run_block_generator block-e5002df2-compressed              1.00    289.9±0.20ms        ? ?/sec    1.10    320.0±0.20ms        ? ?/sec
generator/run_block_generator deep-recursion-plus                    1.00       4.4±0.00s        ? ?/sec    1.65       7.3±0.01s        ? ?/sec
generator/run_block_generator deep-recursion-plus-compressed         1.00       4.4±0.01s        ? ?/sec    1.64       7.3±0.01s        ? ?/sec
generator/run_block_generator duplicate-coin-announce                1.00       4.7±0.00s        ? ?/sec    1.38       6.5±0.01s        ? ?/sec
generator/run_block_generator duplicate-coin-announce-compressed     1.00       4.7±0.00s        ? ?/sec    1.38       6.5±0.01s        ? ?/sec
generator/run_block_generator recursion-pairs                        1.00       3.6±0.01s        ? ?/sec    1.18       4.3±0.00s        ? ?/sec
generator/run_block_generator recursion-pairs-compressed             1.00       3.6±0.00s        ? ?/sec    1.18       4.2±0.00s        ? ?/sec
generator/run_block_generator2 block-1ee588dc                        1.00    130.0±0.05ms        ? ?/sec    1.03    133.4±0.02ms        ? ?/sec
generator/run_block_generator2 block-1ee588dc-compressed             1.00    126.9±0.06ms        ? ?/sec    1.02    130.0±0.02ms        ? ?/sec
generator/run_block_generator2 block-225758                          1.00      7.8±0.01ms        ? ?/sec    1.44     11.2±0.01ms        ? ?/sec
generator/run_block_generator2 block-225758-compressed               1.00      7.8±0.01ms        ? ?/sec    1.44     11.2±0.02ms        ? ?/sec
generator/run_block_generator2 block-4671894                         1.00    441.8±0.75ms        ? ?/sec    1.47    650.7±1.24ms        ? ?/sec
generator/run_block_generator2 block-4671894-compressed              1.00    441.2±0.56ms        ? ?/sec    1.47    650.2±0.76ms        ? ?/sec
generator/run_block_generator2 block-6fe59b24                        1.00    145.0±0.07ms        ? ?/sec    1.03    149.0±0.05ms        ? ?/sec
generator/run_block_generator2 block-6fe59b24-compressed             1.00    141.6±0.08ms        ? ?/sec    1.03    145.3±0.05ms        ? ?/sec
generator/run_block_generator2 block-834752                          1.00     12.3±0.00ms        ? ?/sec    1.05     12.9±0.01ms        ? ?/sec
generator/run_block_generator2 block-834752-compressed               1.00     12.2±0.00ms        ? ?/sec    1.05     12.7±0.00ms        ? ?/sec
generator/run_block_generator2 block-b45268ac                        1.00    128.7±0.04ms        ? ?/sec    1.03    132.0±0.08ms        ? ?/sec
generator/run_block_generator2 block-b45268ac-compressed             1.00    125.6±0.11ms        ? ?/sec    1.02    128.6±0.10ms        ? ?/sec
generator/run_block_generator2 block-c2a8df0d                        1.00    149.1±0.05ms        ? ?/sec    1.02    152.7±0.08ms        ? ?/sec
generator/run_block_generator2 block-c2a8df0d-compressed             1.00    145.2±0.06ms        ? ?/sec    1.02    148.4±0.10ms        ? ?/sec
generator/run_block_generator2 block-e5002df2                        1.00    135.9±0.06ms        ? ?/sec    1.03    139.9±0.10ms        ? ?/sec
generator/run_block_generator2 block-e5002df2-compressed             1.00    132.8±0.04ms        ? ?/sec    1.03    136.2±0.16ms        ? ?/sec
generator/run_block_generator2 deep-recursion-plus                   1.00       4.5±0.01s        ? ?/sec    1.65       7.3±0.00s        ? ?/sec
generator/run_block_generator2 deep-recursion-plus-compressed        1.00       4.4±0.01s        ? ?/sec    1.65       7.3±0.01s        ? ?/sec
generator/run_block_generator2 duplicate-coin-announce               1.00       4.7±0.01s        ? ?/sec    1.39       6.5±0.01s        ? ?/sec
generator/run_block_generator2 duplicate-coin-announce-compressed    1.00       4.7±0.00s        ? ?/sec    1.39       6.6±0.01s        ? ?/sec
generator/run_block_generator2 recursion-pairs                       1.00       3.6±0.01s        ? ?/sec    1.17       4.2±0.00s        ? ?/sec
generator/run_block_generator2 recursion-pairs-compressed            1.00       3.6±0.01s        ? ?/sec    1.17       4.3±0.00s        ? ?/sec
tree-hash/tree-hash block-225758                                     1.00    431.4±0.11µs        ? ?/sec    1.00    430.3±0.05µs        ? ?/sec
tree-hash/tree-hash block-225758-compressed                          1.00    431.3±0.13µs        ? ?/sec    1.00    430.3±0.20µs        ? ?/sec
tree-hash/tree-hash block-4671894                                    1.00     18.5±0.00ms        ? ?/sec    1.00     18.4±0.04ms        ? ?/sec
tree-hash/tree-hash block-4671894-compressed                         1.00     18.5±0.01ms        ? ?/sec    1.00     18.4±0.00ms        ? ?/sec
tree-hash/tree-hash block-834752                                     1.00      7.0±0.00ms        ? ?/sec    1.00      7.0±0.00ms        ? ?/sec
tree-hash/tree-hash block-834752-compressed                          1.00      7.0±0.00ms        ? ?/sec    1.00      7.0±0.00ms        ? ?/sec

Mac OS (M1)

group                                                                after                                  before
-----                                                                -----                                  ------
generator/run_block_generator block-1ee588dc                         1.00     85.9±0.55ms        ? ?/sec    1.06     90.8±0.18ms        ? ?/sec
generator/run_block_generator block-1ee588dc-compressed              1.00     84.3±0.85ms        ? ?/sec    1.08     91.0±0.23ms        ? ?/sec
generator/run_block_generator block-225758                           1.00      2.1±0.02ms        ? ?/sec    1.88      4.0±0.06ms        ? ?/sec
generator/run_block_generator block-225758-compressed                1.00      2.1±0.01ms        ? ?/sec    1.93      4.1±0.02ms        ? ?/sec
generator/run_block_generator block-4671894                          1.00    115.9±0.54ms        ? ?/sec    1.99    230.4±2.35ms        ? ?/sec
generator/run_block_generator block-4671894-compressed               1.00    116.1±0.90ms        ? ?/sec    1.94    225.8±0.35ms        ? ?/sec
generator/run_block_generator block-6fe59b24                         1.00     95.8±0.25ms        ? ?/sec    1.06    102.0±0.19ms        ? ?/sec
generator/run_block_generator block-6fe59b24-compressed              1.00     94.1±0.23ms        ? ?/sec    1.05     98.8±0.15ms        ? ?/sec
generator/run_block_generator block-834752                           1.00      7.5±0.10ms        ? ?/sec    1.02      7.7±0.01ms        ? ?/sec
generator/run_block_generator block-834752-compressed                1.00      7.6±0.15ms        ? ?/sec    1.01      7.6±0.06ms        ? ?/sec
generator/run_block_generator block-b45268ac                         1.00     84.0±0.59ms        ? ?/sec    1.08     90.7±0.23ms        ? ?/sec
generator/run_block_generator block-b45268ac-compressed              1.00     83.0±0.43ms        ? ?/sec    1.08     89.3±0.09ms        ? ?/sec
generator/run_block_generator block-c2a8df0d                         1.00    101.0±0.21ms        ? ?/sec    1.06    107.1±0.11ms        ? ?/sec
generator/run_block_generator block-c2a8df0d-compressed              1.00     99.9±0.10ms        ? ?/sec    1.05    105.2±0.31ms        ? ?/sec
generator/run_block_generator block-e5002df2                         1.00     89.3±0.97ms        ? ?/sec    1.07     95.1±0.21ms        ? ?/sec
generator/run_block_generator block-e5002df2-compressed              1.00     88.8±0.81ms        ? ?/sec    1.07     94.7±0.45ms        ? ?/sec
generator/run_block_generator deep-recursion-plus                    1.00   1128.9±2.08ms        ? ?/sec    1.70   1920.4±2.59ms        ? ?/sec
generator/run_block_generator deep-recursion-plus-compressed         1.00   1128.7±2.65ms        ? ?/sec    1.70   1918.4±2.36ms        ? ?/sec
generator/run_block_generator duplicate-coin-announce                1.00   1166.4±2.52ms        ? ?/sec    1.35   1577.1±2.59ms        ? ?/sec
generator/run_block_generator duplicate-coin-announce-compressed     1.00   1162.6±2.68ms        ? ?/sec    1.36   1576.2±1.33ms        ? ?/sec
generator/run_block_generator recursion-pairs                        1.00    897.7±0.82ms        ? ?/sec    1.18   1063.2±1.04ms        ? ?/sec
generator/run_block_generator recursion-pairs-compressed             1.00    899.0±1.83ms        ? ?/sec    1.18   1059.3±0.98ms        ? ?/sec
generator/run_block_generator2 block-1ee588dc                        1.00     49.2±0.19ms        ? ?/sec    1.03     50.5±0.18ms        ? ?/sec
generator/run_block_generator2 block-1ee588dc-compressed             1.00     48.9±0.28ms        ? ?/sec    1.00     48.9±0.10ms        ? ?/sec
generator/run_block_generator2 block-225758                          1.00      2.2±0.03ms        ? ?/sec    1.45      3.1±0.01ms        ? ?/sec
generator/run_block_generator2 block-225758-compressed               1.00      2.2±0.04ms        ? ?/sec    1.55      3.3±0.01ms        ? ?/sec
generator/run_block_generator2 block-4671894                         1.00    115.9±0.95ms        ? ?/sec    1.61    186.4±2.29ms        ? ?/sec
generator/run_block_generator2 block-4671894-compressed              1.00    115.3±0.95ms        ? ?/sec    1.58    181.7±0.36ms        ? ?/sec
generator/run_block_generator2 block-6fe59b24                        1.00     54.9±1.67ms        ? ?/sec    1.03     56.6±0.12ms        ? ?/sec
generator/run_block_generator2 block-6fe59b24-compressed             1.00     54.2±0.21ms        ? ?/sec    1.01     54.7±0.75ms        ? ?/sec
generator/run_block_generator2 block-834752                          1.02      4.8±0.10ms        ? ?/sec    1.00      4.7±0.06ms        ? ?/sec
generator/run_block_generator2 block-834752-compressed               1.00      4.6±0.08ms        ? ?/sec    1.00      4.6±0.01ms        ? ?/sec
generator/run_block_generator2 block-b45268ac                        1.00     48.5±0.66ms        ? ?/sec    1.01     48.9±0.38ms        ? ?/sec
generator/run_block_generator2 block-b45268ac-compressed             1.00     47.6±0.19ms        ? ?/sec    1.02     48.4±0.32ms        ? ?/sec
generator/run_block_generator2 block-c2a8df0d                        1.00     57.7±0.22ms        ? ?/sec    1.02     58.7±0.15ms        ? ?/sec
generator/run_block_generator2 block-c2a8df0d-compressed             1.00     55.1±0.08ms        ? ?/sec    1.02     56.2±0.32ms        ? ?/sec
generator/run_block_generator2 block-e5002df2                        1.00     51.5±0.12ms        ? ?/sec    1.04     53.4±0.14ms        ? ?/sec
generator/run_block_generator2 block-e5002df2-compressed             1.01     51.1±0.20ms        ? ?/sec    1.00     50.7±0.32ms        ? ?/sec
generator/run_block_generator2 deep-recursion-plus                   1.00   1128.5±3.57ms        ? ?/sec    1.70   1921.8±4.20ms        ? ?/sec
generator/run_block_generator2 deep-recursion-plus-compressed        1.00   1129.0±3.68ms        ? ?/sec    1.70   1921.4±1.90ms        ? ?/sec
generator/run_block_generator2 duplicate-coin-announce               1.00   1165.6±2.81ms        ? ?/sec    1.35   1577.9±0.84ms        ? ?/sec
generator/run_block_generator2 duplicate-coin-announce-compressed    1.00   1163.9±0.85ms        ? ?/sec    1.36   1577.4±1.00ms        ? ?/sec
generator/run_block_generator2 recursion-pairs                       1.00    896.1±1.20ms        ? ?/sec    1.19   1063.7±0.81ms        ? ?/sec
generator/run_block_generator2 recursion-pairs-compressed            1.00    896.5±1.36ms        ? ?/sec    1.19   1069.7±5.61ms        ? ?/sec
tree-hash/tree-hash block-225758                                     1.00    175.2±0.11µs        ? ?/sec    1.00    175.5±0.82µs        ? ?/sec
tree-hash/tree-hash block-225758-compressed                          1.00    175.4±0.31µs        ? ?/sec    1.00    175.5±1.77µs        ? ?/sec
tree-hash/tree-hash block-4671894                                    1.00      7.5±0.02ms        ? ?/sec    1.04      7.8±0.10ms        ? ?/sec
tree-hash/tree-hash block-4671894-compressed                         1.00      7.5±0.00ms        ? ?/sec    1.04      7.8±0.10ms        ? ?/sec
tree-hash/tree-hash block-834752                                     1.00      2.9±0.01ms        ? ?/sec    1.04      3.0±0.04ms        ? ?/sec
tree-hash/tree-hash block-834752-compressed                          1.00      2.9±0.04ms        ? ?/sec    1.01      2.9±0.00ms        ? ?/sec

Copy link

coveralls-official bot commented Feb 2, 2024

Pull Request Test Coverage Report for Build 7836861233

  • -12 of 515 (97.67%) changed or added relevant lines in 9 files are covered.
  • 5 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.2%) to 94.458%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/chia_dialect.rs 13 14 92.86%
src/op_utils.rs 35 36 97.22%
src/run_program.rs 9 10 90.0%
src/traverse_path.rs 79 80 98.75%
src/allocator.rs 309 311 99.36%
src/runtime_dialect.rs 0 6 0.0%
Files with Coverage Reduction New Missed Lines %
src/allocator.rs 1 98.91%
src/run_program.rs 1 97.2%
src/runtime_dialect.rs 3 0.0%
Totals Coverage Status
Change from base Build 7833713031: 0.2%
Covered Lines: 5744
Relevant Lines: 6081

💛 - Coveralls

@Rigidity
Copy link
Contributor

Rigidity commented Feb 3, 2024

This is interesting, do the bench times increase in cases where most of the atoms are large due to the extra branching to determine the type of the NodePtr?

I'm also concerned about returning a reference to a temporary value with atom(), this feels like a technically memory safe, but logically unsafe thing to do.

But otherwise I like this a lot

@arvidn
Copy link
Contributor Author

arvidn commented Feb 3, 2024

This is interesting, do the bench times increase in cases where most of the atoms are large due to the extra branching to determine the type of the NodePtr?

I think there's probably more noise in those timings than one might imagine. I just basically ran it once. But, yeah, there are a few cases where there's more work done. For example, when asking whether a node is a small_int(), I actually construct a small int, if it doesn't use the internal representation but would fit. This is probably not necessary.

There's also some extra work when allocating a new number. There's a check to see if it could be stored as a small int, and if so, it is.

I'm also concerned about returning a reference to a temporary value with atom(), this feels like a technically memory safe, but logically unsafe thing to do.

Yeah, I'm not super happy about this. I think a ring buffer of 16 is more than enough, but maybe it's better to overshoot by more and make it 64 or so.

In the long run I would like to phase out this part of the API though, in favor of visit_node() (and the other getters)

This is still work in progress, as you can see from the TODO comments and missing tests

@arvidn arvidn force-pushed the small-int branch 6 times, most recently from e1913d8 to 3fee725 Compare February 6, 2024 00:53
@arvidn arvidn marked this pull request as ready for review February 6, 2024 01:19
src/more_ops.rs Outdated Show resolved Hide resolved
src/allocator.rs Outdated Show resolved Hide resolved
src/allocator.rs Outdated Show resolved Hide resolved
src/allocator.rs Outdated Show resolved Hide resolved
src/allocator.rs Outdated Show resolved Hide resolved
src/run_program.rs Outdated Show resolved Hide resolved
@richardkiss
Copy link
Contributor

If I understand correctly, this is actually a SmallInt. Maybe we should call it that or SmallAtomInt?

@richardkiss
Copy link
Contributor

This is some good work. The numbers in particular look fantastic. I do worry about subtle consensus changes and wonder if adding more visit_node_as_XXX functions would simplify and alleviate concerns that SmallAtom is always correctly coded whenever it's possible.

Rigidity
Rigidity previously approved these changes Feb 7, 2024
Copy link
Contributor

@Rigidity Rigidity left a comment

Choose a reason for hiding this comment

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

Looks good enough for now on my end, I hope to improve the API a bit to ideally get rid of unsafe and needing to use visitors, but I don't want to hold it back on that

@arvidn
Copy link
Contributor Author

arvidn commented Feb 7, 2024

@Rigidity pointed out that visit_node() could probably return the NodeVisitor, rather than taking a callback. Doing that sounds like an improvement to me. I'll investigate that.

@arvidn
Copy link
Contributor Author

arvidn commented Feb 7, 2024

I applied the suggestion from @Rigidity and made visit_node() return the NodeVisitor rather than taking a callback function to pass it to. I also renamed the function just node().

The change since last version: https://github.com/Chia-Network/clvm_rs/compare/34bbdf08f7d0c59fa3312b66aa1d3173839b6866..e90cd1279ae60da5f21953486c3a454f583ca217

@arvidn arvidn requested a review from Rigidity February 7, 2024 23:16
Rigidity
Rigidity previously approved these changes Feb 7, 2024
Copy link
Contributor

@Rigidity Rigidity left a comment

Choose a reason for hiding this comment

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

lgtm for now

@arvidn arvidn merged commit da3d0bd into main Feb 9, 2024
29 checks passed
@arvidn arvidn deleted the small-int branch February 9, 2024 00:32
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