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

Remove unneded num_bindings in Opcodes and CodeBlock #2967

Merged
merged 7 commits into from
May 27, 2023

Conversation

HalidOdat
Copy link
Member

@HalidOdat HalidOdat commented May 27, 2023

Since the number of bindings is stored in the compiled envionments and when we create the environment we pass it, there is no need to store the num_bindings for the environments

It changes the following:

  • Remove unneeded function_environment_push_location field on CodeBlock
  • Remove unneeded num_bindings field on CodeBlock
  • Remove unneeded num_bindings operand from PushDeclarativeEnvironment opcode
  • Remove unneeded num_bindings operand from PushFunctionEnvironment opcode
  • Make ByteCompiler::push_compile_environment() return only the index.
  • Remove num_bindings field from environment stack methods that push environments.
  • Move CodeBlock flags to bitflags field.

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

Test262 conformance changes

Test result main count PR count difference
Total 93,990 93,990 0
Passed 74,620 74,620 0
Ignored 17,622 17,622 0
Failed 1,748 1,748 0
Panics 0 0 0
Conformance 79.39% 79.39% 0.00%

@codecov
Copy link

codecov bot commented May 27, 2023

Codecov Report

Merging #2967 (988d453) into main (15749ed) will decrease coverage by 0.01%.
The diff coverage is 77.85%.

@@            Coverage Diff             @@
##             main    #2967      +/-   ##
==========================================
- Coverage   50.02%   50.01%   -0.01%     
==========================================
  Files         446      446              
  Lines       45896    45906      +10     
==========================================
+ Hits        22959    22961       +2     
- Misses      22937    22945       +8     
Impacted Files Coverage Δ
boa_cli/src/debug/function.rs 0.00% <0.00%> (ø)
boa_engine/src/vm/flowgraph/mod.rs 0.00% <ø> (ø)
boa_engine/src/vm/opcode/mod.rs 45.45% <ø> (ø)
boa_engine/src/vm/opcode/call/mod.rs 22.89% <50.00%> (ø)
boa_engine/src/vm/code_block.rs 55.07% <54.34%> (-0.65%) ⬇️
boa_engine/src/bytecompiler/class.rs 15.60% <56.25%> (-0.20%) ⬇️
boa_engine/src/bytecompiler/declarations.rs 55.44% <80.00%> (-0.09%) ⬇️
boa_engine/src/builtins/eval/mod.rs 65.65% <100.00%> (-0.35%) ⬇️
boa_engine/src/bytecompiler/env.rs 93.33% <100.00%> (ø)
boa_engine/src/bytecompiler/function.rs 90.76% <100.00%> (-0.14%) ⬇️
... and 12 more

... and 11 files with indirect coverage changes

@HalidOdat HalidOdat requested a review from a team May 27, 2023 06:17
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 great.

boa_engine/src/vm/code_block.rs Outdated Show resolved Hide resolved
@raskad raskad requested a review from a team May 27, 2023 14:59
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.

I like the changes a lot! I'd second the above review about docs. Other than that, great work! 😄

@raskad raskad added this pull request to the merge queue May 27, 2023
Merged via the queue into main with commit 67c5652 May 27, 2023
@raskad raskad deleted the remove-unneded-num_bindings branch May 27, 2023 22:09
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 technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants