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

Direct conversion from u8 to Opcode #2951

Merged
merged 1 commit into from
May 31, 2023
Merged

Direct conversion from u8 to Opcode #2951

merged 1 commit into from
May 31, 2023

Conversation

HalidOdat
Copy link
Member

@HalidOdat HalidOdat commented May 20, 2023

Optimizes opcode fetching by removing the checks that validate if it's a correct opcode. every u8 combination matches to a valid Opcode variant (even if it's reserved).

Main:

Richards: 20.5
DeltaBlue: 24.8
RayTrace: 81.0
----
Score (version 7): 34.5
undefined

PR:

Richards: 20.6
DeltaBlue: 24.8
RayTrace: 82.1
----
Score (version 7): 34.8
undefined

There is a small improvement in performance.

@HalidOdat HalidOdat added performance Performance related changes and issues execution Issues or PRs related to code execution labels May 20, 2023
@HalidOdat HalidOdat added this to the v0.17.0 milestone May 20, 2023
@github-actions
Copy link

github-actions bot commented May 20, 2023

Test262 conformance changes

Test result main count PR count difference
Total 94,118 94,118 0
Passed 74,620 74,620 0
Ignored 17,750 17,750 0
Failed 1,748 1,748 0
Panics 0 0 0
Conformance 79.28% 79.28% 0.00%

@HalidOdat HalidOdat force-pushed the optimization/opcode-fetch branch from b2f4eed to e0b97c4 Compare May 20, 2023 12:28
@codecov
Copy link

codecov bot commented May 20, 2023

Codecov Report

Merging #2951 (e0b97c4) into main (44ef49e) will increase coverage by 0.34%.
The diff coverage is 66.66%.

❗ Current head e0b97c4 differs from pull request most recent head bb4d02f. Consider uploading reports for the commit bb4d02f to get more accurate results

@@            Coverage Diff             @@
##             main    #2951      +/-   ##
==========================================
+ Coverage   50.05%   50.40%   +0.34%     
==========================================
  Files         446      444       -2     
  Lines       45975    45355     -620     
==========================================
- Hits        23015    22859     -156     
+ Misses      22960    22496     -464     
Impacted Files Coverage Δ
boa_engine/src/vm/code_block.rs 55.80% <0.00%> (-0.51%) ⬇️
boa_engine/src/vm/flowgraph/mod.rs 0.00% <0.00%> (ø)
boa_engine/src/vm/opcode/nop/mod.rs 0.00% <ø> (ø)
boa_engine/src/vm/opcode/mod.rs 53.33% <80.00%> (+7.87%) ⬆️
boa_engine/src/vm/mod.rs 54.16% <100.00%> (+0.27%) ⬆️

... and 80 files with indirect coverage changes

Copy link
Member

@jasonwilliams jasonwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@HalidOdat HalidOdat force-pushed the optimization/opcode-fetch branch from e0b97c4 to 279ec1a Compare May 25, 2023 14:31
@raskad
Copy link
Member

raskad commented May 27, 2023

I think we don't need transmute here to get the performance improvement. Even on opt-level 1 the produced code is the same when all u8s are matched to their enum variants: https://rust.godbolt.org/z/oxMeMMrjG.

We should also remove the Opcode::from_raw function since that can be done with into() safeley now.

@HalidOdat HalidOdat force-pushed the optimization/opcode-fetch branch from 279ec1a to bb4d02f Compare May 30, 2023 13:31
@HalidOdat HalidOdat requested a review from raskad May 30, 2023 13:32
Copy link
Member

@raskad raskad 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

@raskad raskad added this pull request to the merge queue May 31, 2023
Merged via the queue into main with commit f0422bd May 31, 2023
@jedel1043 jedel1043 deleted the optimization/opcode-fetch branch May 31, 2023 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
execution Issues or PRs related to code execution performance Performance related changes and issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants