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

Reduce mem allocations #729

Merged
merged 7 commits into from
Jan 25, 2025
Merged

Conversation

vprigent
Copy link
Contributor

@vprigent vprigent commented Jan 16, 2025

As mentioned below, I'm unsure as to what is making the tests fail.

Benchmark:
Using a local projects page firing 42 queries and detecting one include missing:

On 8.0.0

allocated memory by gem
-----------------------------------
   3598454  activesupport-8.0.0.1
   1275662  bullet/lib

Patch:

allocated memory by gem
-----------------------------------
   3598454  activesupport-8.0.0.1
   1139414  bullet/lib

Running the benchmark:

8.0.0

1000 Posts with 10000 Comments and 100 Users       13.591269   0.893714  14.484983 ( 14.682978)

Patch:

1000 Posts with 10000 Comments and 100 Users        6.464706   0.390424   6.855130 (  7.152605)

Which is a surprising improvement, considering I was mostly aiming at reducing the memory usage. I suspect this will actually be more valuable than the 5% memory usage reduction for most people.

Happy for you to review the changes @flyerhzm
I'm not sure how to solve the specs all failing at this time.

I believe there's still a fair bit on the table within the naive implementation for grabbing stack traces, we could for a start limit the size of the stack trace we grab by using caller(0..n) over Thread.current.backtrace or filter straight as we grab the content of the backtrace might do the trick too?

@vprigent vprigent closed this Jan 16, 2025
@vprigent vprigent reopened this Jan 16, 2025
@vprigent vprigent marked this pull request as draft January 16, 2025 02:16
def bullet_class_name
sub(/:[^:]*?$/, '')
@bullet_class_name ||= begin
last_colon = self.rindex(':')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

microbenchmark on sub showed rindex is about twice as fast for this sort of work.
memoizing here is just the cherry on top

Its causing all the specs to fail, however I was not able to figure out 
why it was doing so...
I don't believe primary_key is ever changed within a request
@vprigent vprigent force-pushed the reduce-mem-allocations branch from 3f8ff37 to f9677ab Compare January 21, 2025 20:59
@vprigent
Copy link
Contributor Author

I haven't changed anything relevant to the CI and each failed job seems to relate to a CI change rather than the code changes here... Not sure what's up with that.

@vprigent vprigent marked this pull request as ready for review January 22, 2025 04:39
@flyerhzm
Copy link
Owner

@vprigent great job on performance tuning, thank you, I'll merge it and fix the CI.

@flyerhzm flyerhzm merged commit f475e37 into flyerhzm:main Jan 25, 2025
3 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants