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 weak references in descendants tracker #31442

Merged
merged 1 commit into from Mar 26, 2019

Conversation

Projects
None yet
6 participants
@ebeigarts
Copy link
Contributor

ebeigarts commented Dec 14, 2017

This PR will make descendants tracker use weak references and weak map, so that unused subclasses (unused anonymous subclasses) can be garbage collected, for more details you can see #31395. /cc @matthewd

Ideally we would need WeekKeyMap and WeekSet/WeekArray, but the ref gem only provides WeekKeyMap.

Fixes #31395.

Benchmark results:

Original:
              Parent    352.629k (± 6.2%) i/s -      1.774M in   5.052777s
               Child    661.191k (± 6.6%) i/s -      3.329M in   5.062953s
          Grandchild      1.834M (± 5.0%) i/s -      9.151M in   5.001540s

           Class.new    348.889k (±29.5%) i/s -      1.443M in   5.070915s

Weak map (ref gem):
              Parent    131.917k (± 4.9%) i/s -    664.962k in   5.054227s
               Child    212.329k (±12.9%) i/s -      1.058M in   5.081918s
          Grandchild    848.726k (±14.1%) i/s -      4.152M in   5.013853s

           Class.new     75.910k (±29.9%) i/s -    280.782k in   5.172886s

Weak ref array:
              Parent    261.569k (± 6.5%) i/s -      1.314M in   5.045141s
               Child    510.892k (± 4.6%) i/s -      2.581M in   5.064146s
          Grandchild      1.813M (± 4.8%) i/s -      9.048M in   5.002708s

           Class.new      3.376k (±186.0%) i/s -      6.182k in   5.028783s

According to this class creation would be 4.5x times slower and .descendants calls would be 2.6x times slower. The performance could be improved by writing WeekSet and maybe improving WeekKeyMap.

Benchmark examples:

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Dec 14, 2017

This isn't worth adding a dependency for. I thought the plan was to use WeakRef?

@ebeigarts

This comment has been minimized.

Copy link
Contributor Author

ebeigarts commented Dec 14, 2017

The ref gem uses WeakRef underhood for MRI.

Using just WeakRef there could be situations where the descendants array contains a lot of dead WeakRef's (and would eat up small amount of memory) and we would need to somehow cleanup the array of those dead references (which ref does for maps with finalizers) or we could just leave them there and when calling descendants we would just skip the dead references.

I can also try to implement DescendantsMap without using ref gem (~160 lines).

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Dec 14, 2017

I'd like to try for something more like the array-of-weakref. As long as we're aware of threading concerns, we should be able to clean it out when adding new entries. That leaves a degenerate case where it doesn't shrink if no new descendants are being defined... but at that point the list isn't growing either.

@ebeigarts

This comment has been minimized.

Copy link
Contributor Author

ebeigarts commented Dec 14, 2017

Sounds good enough, I will try that.

@ebeigarts ebeigarts force-pushed the ebeigarts:weak_descendants_tracker branch Dec 15, 2017

@ebeigarts

This comment has been minimized.

Copy link
Contributor Author

ebeigarts commented Dec 15, 2017

@matthewd I have updated the implementation to use WeakRef.

Benchmarks:

              Parent    261.569k (± 6.5%) i/s -      1.314M in   5.045141s
               Child    510.892k (± 4.6%) i/s -      2.581M in   5.064146s
          Grandchild      1.813M (± 4.8%) i/s -      9.048M in   5.002708s

           Class.new      3.376k (±186.0%) i/s -      6.182k in   5.028783s

Class.new benchmark is much slower because if the descendants array is large, then the cleaning of the dead refs take a lot of time.

Without the cleanup! it is

           Class.new    111.620k (±44.2%) i/s -    443.080k in  10.032368s
@ebeigarts

This comment has been minimized.

Copy link
Contributor Author

ebeigarts commented Dec 15, 2017

There is one case, where this will not work:

class Parent; extend ActiveSupport::DescendantsTracker end
child_klass = Class.new(Parent)
grandchild_klass = Class.new(child_klass)

In this example child_klass will not be garbage collected, because child_klass is registred as the key in @@direct_descendants, because it had descendants.

I think most people will not inherit an anonymous class from another anonymous class.

@ebeigarts ebeigarts force-pushed the ebeigarts:weak_descendants_tracker branch 2 times, most recently to b2d4118 Dec 15, 2017

@FutoRicky

This comment has been minimized.

Copy link

FutoRicky commented May 9, 2018

Any update on this?

@simi

This comment has been minimized.

Copy link
Contributor

simi commented Mar 25, 2019

Hello. I'm also interested in this fix since if I understand it well it allows you to safely use anonymous ActiveRecord::Base inherits which can be handy for temporary tables.

Is there anything I can help with?

@ebeigarts ebeigarts force-pushed the ebeigarts:weak_descendants_tracker branch from b2d4118 to ea8f8ed Mar 25, 2019

@ebeigarts

This comment has been minimized.

Copy link
Contributor Author

ebeigarts commented Mar 25, 2019

I have rebased this against master. @matthewd can you take a look at this?

@matthewd
Copy link
Member

matthewd left a comment

Thanks for your patience, and sorry this fell off the radar 😔

If you can just make codeclimate happy (with our minimum ruby, do/rescue/end is now valid), I think it's good to :shipit:

@ebeigarts ebeigarts force-pushed the ebeigarts:weak_descendants_tracker branch from ea8f8ed to 8f7f9e4 Mar 25, 2019

@ebeigarts

This comment has been minimized.

Copy link
Contributor Author

ebeigarts commented Mar 25, 2019

@matthewd codeclimate should be happy now :)

@simi

This comment has been minimized.

Copy link
Contributor

simi commented Mar 25, 2019

I think this is worth changelog entry as well.

@ebeigarts

This comment has been minimized.

Copy link
Contributor Author

ebeigarts commented Mar 25, 2019

@simi @matthewd yeah, changelog would be nice, do I need to update changelog myself or this will be done by the rails core? (I think it's not mentioned in the CONTRIBUTING.md)

@simi

This comment has been minimized.

Copy link
Contributor

simi commented Mar 25, 2019

I think it is common to write changelog entry on your own.

Use weak references in descendants tracker
It allows anonymous subclasses to be garbage collected.

@ebeigarts ebeigarts force-pushed the ebeigarts:weak_descendants_tracker branch from 8f7f9e4 to 7432c92 Mar 26, 2019

@ebeigarts

This comment has been minimized.

Copy link
Contributor Author

ebeigarts commented Mar 26, 2019

I have updated the changelog, @matthewd can we merge this?

@matthewd matthewd merged commit 3296dc2 into rails:master Mar 26, 2019

3 checks passed

buildkite/rails Build #59869 passed (19 minutes, 44 seconds)
Details
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Mar 26, 2019

Thanks again for persevering on this! (And for the poke, @simi)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.