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

Cache this value #3771

Merged
merged 1 commit into from
Mar 28, 2024
Merged

Cache this value #3771

merged 1 commit into from
Mar 28, 2024

Conversation

HalidOdat
Copy link
Member

@HalidOdat HalidOdat commented Mar 27, 2024

The value of this when this is used in a function does not change after the initial environment chain search, so now we cache this value. This prevents the this value from being searched in the environment chain on every this usage.

There is a small increase in performance, the functions that use this a lot are the ones that benefit from this optimization.

Benchmarks

Main

PROGRESS Richards
RESULT Richards 58.0
PROGRESS DeltaBlue
RESULT DeltaBlue 63.9
PROGRESS Encrypt
PROGRESS Decrypt
RESULT Crypto 69.9
PROGRESS RayTrace
RESULT RayTrace 198
PROGRESS Earley
PROGRESS Boyer
RESULT EarleyBoyer 181
PROGRESS Splay
RESULT Splay 192
PROGRESS NavierStokes
RESULT NavierStokes 150
SCORE 115

PR

PROGRESS Richards
RESULT Richards 60.7
PROGRESS DeltaBlue
RESULT DeltaBlue 65.0
PROGRESS Encrypt
PROGRESS Decrypt
RESULT Crypto 71.3
PROGRESS RayTrace
RESULT RayTrace 200
PROGRESS Earley
PROGRESS Boyer
RESULT EarleyBoyer 183
PROGRESS Splay
RESULT Splay 194
PROGRESS NavierStokes
RESULT NavierStokes 153
SCORE 117

@HalidOdat HalidOdat added the performance Performance related changes and issues label Mar 27, 2024
@HalidOdat HalidOdat added this to the v0.18.1 milestone Mar 27, 2024
@HalidOdat HalidOdat requested a review from a team March 27, 2024 23:04
Copy link

Test262 conformance changes

Test result main count PR count difference
Total 50,268 50,268 0
Passed 42,773 42,773 0
Ignored 1,391 1,391 0
Failed 6,104 6,104 0
Panics 18 18 0
Conformance 85.09% 85.09% 0.00%

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.

Nice work!

Thought: I think in the future we could investigate if inverting the logic so that all contexts "assume" there's a this cached instead of the opposite is better; maybe using a this register on the VM that is pushed and popped from the stack when entering a new context. It would be interesting to see how many things break with this approach.

Copy link
Member

@nekevss nekevss 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! 😄

Edit: Second Jedel's thought

@nekevss nekevss added this pull request to the merge queue Mar 27, 2024
Merged via the queue into main with commit fb8e16b Mar 28, 2024
13 checks passed
@HalidOdat HalidOdat deleted the optimization/cached-this-value branch March 28, 2024 00:18
@raskad raskad modified the milestones: v0.18.1, v0.19.0 Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance related changes and issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants