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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

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 51.87%. Comparing base (6ddc2b4) to head (346b42f).
Report is 244 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%   51.87%   +4.62%     
==========================================
  Files         476      468       -8     
  Lines       46892    45307    -1585     
==========================================
+ Hits        22154    23501    +1347     
+ Misses      24738    21806    -2932     

☔ 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 Outdated Show resolved Hide resolved
Co-authored-by: Haled Odat <8566042+HalidOdat@users.noreply.github.com>
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