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

Introduce new more efficient LittleEndianConvert trait #290

Merged
merged 1 commit into from Dec 29, 2021

Conversation

Robbepop
Copy link
Collaborator

@Robbepop Robbepop commented Dec 29, 2021

This version is more efficient, smaller and more elegant than the old version.
Unfortunately this is a breaking change for implementers of the old LittleEndianConvert trait, however, since wasmi provided implementations for all necessary types this breakage will possibly be alright.

The new trait definition was a result of the work from #287.
Merging this PR will make the common base of both v0 and v1 implementations of wasmi slightly bigger.

Benchmarks

There was no noticeable difference between the two versions, however, I think that's partially due to the fact that our benchmarks do not really test out the memory accesses in depth.

Before

test bench_regex_redux ... bench:   1,807,313 ns/iter (+/- 73,280)
test bench_rev_comp    ... bench:   1,637,458 ns/iter (+/- 60,874)
test bench_tiny_keccak ... bench:   1,294,044 ns/iter (+/- 59,388)
test fac_opt           ... bench:      21,822 ns/iter (+/- 2,109)
test fac_recursive     ... bench:      23,316 ns/iter (+/- 3,726)
test recursive_ok      ... bench:     601,160 ns/iter (+/- 63,188)
test recursive_trap    ... bench:      78,184 ns/iter (+/- 10,151)

After

test bench_regex_redux ... bench:   1,790,866 ns/iter (+/- 168,485)
test bench_rev_comp    ... bench:   1,633,503 ns/iter (+/- 106,277)
test bench_tiny_keccak ... bench:   1,246,634 ns/iter (+/- 49,975)
test fac_opt           ... bench:      22,422 ns/iter (+/- 674)
test fac_recursive     ... bench:      24,096 ns/iter (+/- 1,068)
test recursive_ok      ... bench:     601,510 ns/iter (+/- 26,461)
test recursive_trap    ... bench:      78,344 ns/iter (+/- 7,130)

@codecov-commenter
Copy link

Codecov Report

Merging #290 (d3e0958) into master (aea0581) will increase coverage by 0.38%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #290      +/-   ##
==========================================
+ Coverage   79.62%   80.00%   +0.38%     
==========================================
  Files          30       30              
  Lines        4756     4672      -84     
==========================================
- Hits         3787     3738      -49     
+ Misses        969      934      -35     
Impacted Files Coverage Δ
src/memory/mod.rs 76.32% <100.00%> (+0.45%) ⬆️
src/value.rs 68.84% <100.00%> (+2.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aea0581...d3e0958. Read the comment docs.

@Robbepop Robbepop merged commit 9b1bfbb into master Dec 29, 2021
@athei athei deleted the rf-new-little-endian-convert branch July 26, 2022 10:44
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

3 participants