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

use with_capacity to reduce re-allocations fixes #3896 #3961

Merged
merged 5 commits into from
Nov 2, 2024

Conversation

jasonwilliams
Copy link
Member

@jasonwilliams jasonwilliams commented Aug 19, 2024

This PR attempts to address #3896

We already raised the threshold, but it would be good to not re-allocate the hashmaps and vectors as we will up the property storage during startup.

It doesn't seem to have much noticeable affect in the performance profiler but DHAT shows less grow_one calls from boa_engine::object::shape::property_table::PropertyTableInner::insert (object/shape/property_table.rs:70:9), there are still some, we may be able to find these by checking .len() == .capacity() and tracing back where these are coming from

@jasonwilliams jasonwilliams changed the title use with_capacity to reduce re-allocations use with_capacity to reduce re-allocations fixes #3896 Aug 19, 2024
@jasonwilliams jasonwilliams added performance Performance related changes and issues Internal Category for changelog labels Aug 19, 2024
Copy link

codecov bot commented Aug 19, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Project coverage is 52.88%. Comparing base (6ddc2b4) to head (1882c65).
Report is 279 commits behind head on main.

Files with missing lines Patch % Lines
core/engine/src/context/icu.rs 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3961      +/-   ##
==========================================
+ Coverage   47.24%   52.88%   +5.64%     
==========================================
  Files         476      483       +7     
  Lines       46892    46855      -37     
==========================================
+ Hits        22154    24780    +2626     
+ Misses      24738    22075    -2663     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

I'd be for merging this. I ran the benchmarks locally and there are no significant changes overall.

@raskad raskad added this to the next-release milestone Sep 10, 2024
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.

Besides the comment, this looks good to me! :)

core/engine/src/builtins/array/mod.rs Show resolved Hide resolved
@jasonwilliams jasonwilliams added this pull request to the merge queue Nov 2, 2024
Merged via the queue into boa-dev:main with commit d8ec97c Nov 2, 2024
14 checks passed
@jasonwilliams jasonwilliams deleted the set-capacity branch November 2, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Category for changelog performance Performance related changes and issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants