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

Add Memory::data_and_store_mut #448

Merged
merged 3 commits into from Sep 17, 2022
Merged

Conversation

AldaronLau
Copy link
Contributor

Solves #447

@paritytech-cicd-pr
Copy link

paritytech-cicd-pr commented Sep 17, 2022

CRITERION BENCHMARKS

BENCHMARK MASTER PR Diff
compile_and_validate_v1 5.4511 ms 5.4661 ms ⚪ +0.3055%
execute_count_until_v1 2.0232 ms 2.0254 ms ⚪ +0.2418%
execute_factorial_iterative_v1 922.66 ns 918.52 ns ⚪ -0.3802%
execute_factorial_recursive_v1 1.2555 µs 1.2438 µs ⚪ -0.9251%
execute_fib_iterative_v1 4.8057 ms 4.8006 ms ⚪ -0.0511%
execute_fib_recursive_v1 11.118 ms 11.138 ms ⚪ +0.1925%
execute_global_bump_v1 2.8163 ms 2.8169 ms ⚪ -0.0013%
execute_host_calls_v1 31.242 µs 30.778 µs 🟢 -1.9683%
execute_memory_fill_v1 4.0545 ms 4.0526 ms ⚪ -0.0928%
execute_memory_sum_v1 3.8005 ms 3.7980 ms ⚪ -0.0698%
execute_memory_vec_add_v1 8.0462 ms 8.0624 ms ⚪ +0.1796%
execute_recursive_is_even_v1 2.2401 ms 2.2480 ms ⚪ +0.2698%
execute_recursive_ok_v1 295.35 µs 295.38 µs ⚪ +0.0058%
execute_recursive_scan_v1 357.15 µs 357.30 µs ⚪ +0.0195%
execute_recursive_trap_v1 25.477 µs 25.378 µs ⚪ -0.2265%
execute_regex_redux_v1 1.3864 ms 1.3847 ms ⚪ -0.1566%
execute_rev_complement_v1 1.3832 ms 1.3861 ms ⚪ +0.1524%
execute_tiny_keccak_v1 1.2303 ms 1.2353 ms ⚪ +0.4158%
execute_trunc_f2i_v1 1.8031 ms 1.8036 ms ⚪ +0.0586%
instantiate_v1 73.660 µs 73.604 µs ⚪ +0.0750%

Link to pipeline

@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2022

Codecov Report

Merging #448 (fa79712) into master (e908d52) will decrease coverage by 0.10%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #448      +/-   ##
==========================================
- Coverage   79.54%   79.44%   -0.11%     
==========================================
  Files          71       71              
  Lines        6185     6193       +8     
==========================================
  Hits         4920     4920              
- Misses       1265     1273       +8     
Impacted Files Coverage Δ
wasmi_v1/src/memory/mod.rs 58.47% <0.00%> (-1.53%) ⬇️
wasmi_v1/src/store.rs 70.54% <0.00%> (-2.85%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

comments needs some small fix but besides that looks good to me! Thanks a lot for the PR!

Comment on lines 332 to 333
/// Returns an exclusive slice to the bytes underlying to the byte buffer, and a shared
/// reference to the user provided state.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Returns an exclusive slice to the bytes underlying to the byte buffer, and a shared
/// reference to the user provided state.
/// Returns an exclusive slice to the bytes underlying to the [`Memory`], and an exclusive
/// reference to the user provided state.

Comment on lines 302 to 303
/// Returns an exclusive reference to the associated entity of the linear memory and a shared
/// reference to the user provided state.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Returns an exclusive reference to the associated entity of the linear memory and a shared
/// reference to the user provided state.
/// Returns an exclusive reference to the associated entity of the linear memory and an exclusive
/// reference to the user provided state.

@AldaronLau
Copy link
Contributor Author

I pushed the comment fix; I'm wondering if the Memory::data() and Memory::data_mut() comments need to be changed as well;

Returns a shared slice to the bytes underlying to the byte buffer.

Returns an exclusive slice to the bytes underlying to the byte buffer.

If they were to match, it would say:

Returns an shared slice to the bytes underlying to the [`Memory`].

Returns an exclusive slice to the bytes underlying to the [`Memory`].

Also, I'm thinking grammatically, it would make sense to remove "to"

@Robbepop
Copy link
Collaborator

Want to fix the other comments before we merge this? I am also fine just merging.

@AldaronLau
Copy link
Contributor Author

Sure, I can add the fixes.

@Robbepop
Copy link
Collaborator

Thanks a lot for the new API @AldaronLau !

@Robbepop Robbepop merged commit 753b05d into wasmi-labs:master Sep 17, 2022
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.

None yet

4 participants