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

Refactor environment stack to remove some panics #3893

Merged
merged 3 commits into from
Jul 10, 2024

Conversation

raskad
Copy link
Member

@raskad raskad commented Jul 6, 2024

Thie PR refactors the environment stack to avoid panics. The main change is to put the global declarative environment in a separete field instead of on the stack itself. That way we can safeley avoid panics.

There was only a very slight difference in benchmarks, but I think reducing the panics is worth it either way.
After running the benchmarks a few times they look pretty good:

Before:

RESULT Richards 60.6
RESULT DeltaBlue 63.4
RESULT Crypto 74.4
RESULT RayTrace 202
RESULT EarleyBoyer 186
RESULT RegExp 61.1
RESULT Splay 199
RESULT NavierStokes 175
SCORE 111

After:

RESULT Richards 65.8
RESULT DeltaBlue 69.6
RESULT Crypto 77.1
RESULT RayTrace 201
RESULT EarleyBoyer 199
RESULT RegExp 59.9
RESULT Splay 226
RESULT NavierStokes 176
SCORE 116

@raskad raskad added performance Performance related changes and issues execution Issues or PRs related to code execution labels Jul 6, 2024
Copy link

github-actions bot commented Jul 6, 2024

Test262 conformance changes

Test result main count PR count difference
Total 48,212 48,212 0
Passed 42,875 42,875 0
Ignored 1,413 1,413 0
Failed 3,924 3,924 0
Panics 0 0 0
Conformance 88.93% 88.93% 0.00%

@raskad raskad requested a review from a team July 6, 2024 21:46
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Really nice enhancement!

core/engine/src/environments/runtime/mod.rs Outdated Show resolved Hide resolved
@jedel1043 jedel1043 requested a review from a team July 10, 2024 14:48
Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

Nice work, looks good to me! :)

@jedel1043 jedel1043 added this pull request to the merge queue Jul 10, 2024
Merged via the queue into main with commit ab0854f Jul 10, 2024
13 checks passed
@jedel1043 jedel1043 deleted the refactor-environment-stack-remove-panics branch July 10, 2024 15:46
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