chore: fix JSONEntry Eq and add documentation #61
Conversation
jialinli98
force-pushed
the
jl/documentation
branch
7 times, most recently
from
b4f1f9b to
0ac4c02
Compare
jialinli98
force-pushed
the
jl/documentation
branch
from
0ac4c02 to
246e245
Compare
jialinli98
force-pushed
the
jl/documentation
branch
from
490ec99 to
c29d179
Compare
jialinli98
force-pushed
the
jl/documentation
branch
4 times, most recently
from
7dc7bda to
e213651
Compare
jialinli98
force-pushed
the
jl/documentation
branch
2 times, most recently
from
1d59267 to
614278e
Compare
jialinli98
force-pushed
the
jl/documentation
branch
from
614278e to
8a985b5
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'ACIR Opcodes'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.01.
| Benchmark suite | Current: a2487c0 | Previous: 33bf554 | Ratio |
|---|---|---|---|
parse_json_from_string_JSON16kb_Bench.json/main |
1121187 acir_opcodes |
1098668 acir_opcodes |
1.02 |
parse_json_from_string_JSON512b_Bench.json/main |
38874 acir_opcodes |
38179 acir_opcodes |
1.02 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Circuit Size'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.01.
| Benchmark suite | Current: a2487c0 | Previous: 33bf554 | Ratio |
|---|---|---|---|
parse_json_from_string_JSON16kb_Bench.json/main |
1533190 circuit_size |
1473490 circuit_size |
1.04 |
parse_json_from_string_JSON512b_Bench.json/main |
70594 circuit_size |
68763 circuit_size |
1.03 |
This comment was automatically generated by workflow using github-action-benchmark.
src/json.nr
Outdated
| let mut push_transcript = push_transcript; | ||
| let mut scan_token = scan_token; | ||
| let mut increase_length = increase_length; |
There was a problem hiding this comment.
These mutable variables are not used.
| // If the current token creates an object or array, subsequent entries will be a child of this object | ||
| // i.e. we need to assign them a new identifier so increase `next_identity_value` | ||
| next_identity_value = next_identity_value + is_start_of_object_or_array; | ||
| std::as_witness(next_identity_value); |
There was a problem hiding this comment.
next_identity_value is being unconditionally incremented here which doesn't match up with the documentation.
There was a problem hiding this comment.
I think next_identity_value is only incremented when is_start_of_object_or_array is 1, so only at the beginning of objects or arrays.
There was a problem hiding this comment.
The comment implies that this should only happen if !preserve_num_entries which isn't the case here.
There was a problem hiding this comment.
The comment is correct but misleading. preserve_num_entries is mutually exclusive with is_start_of_object_or_array and is_end_of_object_or_array:
When is_start_of_object_or_array or is_end_of_object_or_array = 1 -> preserve_num_entries = 0
When preserve_num_entries = 1 -> is_start_of_object_or_array = 0, is_end_of_object_or_array = 0
So the if !preserve_num_entries check is redundant.
When preserve_num_entries = 1, both flags are 0, making this a no-op. I'll update the comment
|
I've fixed the merge that I busted. |
jialinli98
force-pushed
the
jl/documentation
branch
from
8bb74fc to
9ea58ab
Compare
There was a problem hiding this comment.
This PR has a lot of innocuous changes (documentation, Eq impl change, etc.) mixed in with unexplained changes to business logic in create_json_entries. The large diff created by all the new documentation and restructuring of code into blocks makes it harder to see what's a meaningful change or just code reorganization.
src/json.nr
Outdated
|
|
||
| // 3.5 gates | ||
| self.key_data[cast_num_to_u32(entry_ptr)] = new_key_data * create_json_entry; | ||
|
|
||
| // 3.5 gates | ||
| parent_context_stack[cast_num_to_u32(depth)] = new_context_stack_entry; | ||
|
|
||
| // 4.5 gates | ||
| self.json_entries_packed[cast_num_to_u32(entry_ptr)] = | ||
| JSONEntryPacked { value: new_entry * create_json_entry }; | ||
| self.key_data[entry_ptr] = new_key_data * create_json_entry as Field; | ||
|
|
||
| // Update `entry_ptr` (points to the head of self.key_data and self.json_entries_packed) | ||
| // 1 gate | ||
| next_identity_value = next_identity_value + is_start_of_object_or_array; | ||
| std::as_witness(next_identity_value); | ||
| entry_ptr += create_json_entry as u32; | ||
|
|
||
| // 1 gate | ||
| depth = depth + is_start_of_object_or_array - is_end_of_object_or_array; | ||
|
|
||
| // 1 gate | ||
| // 2105 + 46.25 | ||
| // subtotal 66.75? | ||
| entry_ptr += create_json_entry; | ||
| std::as_witness(entry_ptr); |
There was a problem hiding this comment.
I don't understand why this is changing.
There was a problem hiding this comment.
There’s no logic change in this function. Zac reorganized the code to group related lines together while adding comments for clarity. Some lines, like incrementing depth and setting parent_context_stack, were moved up. Let me know if anything still seems off.
Description
Problem*
closes #53
closes #52
closes #60
Zac's commit to add documentation: f4edfab
Summary*
Additional Context
PR Checklist*
cargo fmton default settings.