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

Improve object shape friendliness #20

Merged
merged 5 commits into from
Jan 7, 2024

Conversation

natematykiewicz
Copy link
Contributor

@natematykiewicz natematykiewicz commented Jan 7, 2024

I'm mimicking what Jean did here:
rails/rails#47023
rails/rails#47032
rails/rails#47049

Otherwise, if you've got multiple associated objects on a single class, you can end up with a lot of permutations of object shapes because the instance variables are lazily defined in whatever order the associated objects may or may not have been called in.

Author has these 3 instance variables:

  1. @archiver
  2. @classified
  3. @fortification

That leaves you with these permutations of object shapes:

  1. (none called)
  2. @archiver
  3. @archiver @classified
  4. @archiver @fortification
  5. @archiver @classified @fortification
  6. @archiver @fortification @classified
  7. @classified
  8. @classified @archiver
  9. @classified @fortification
  10. @classified @archiver @fortification
  11. @classified @fortification @archiver
  12. @fortification
  13. @fortification @archiver
  14. @fortification @classified
  15. @fortification @archiver @classified
  16. @fortification @classified @archiver

Now, we'll have 1 shape:

  1. @archiver @classified @fortification

Copy link
Owner

@kaspth kaspth left a comment

Choose a reason for hiding this comment

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

Sounds good! What if we use a single @associated_objects hash ivar?

We can initialize it to nil in init_internals and then assign it in a reader method?

lib/active_record/associated_object/object_association.rb Outdated Show resolved Hide resolved
lib/active_record/associated_object/object_association.rb Outdated Show resolved Hide resolved
lib/active_record/associated_object/object_association.rb Outdated Show resolved Hide resolved
lib/active_record/associated_object/object_association.rb Outdated Show resolved Hide resolved
lib/active_record/associated_object/object_association.rb Outdated Show resolved Hide resolved
@natematykiewicz
Copy link
Contributor Author

I considered that, but didn't want to add a Hash allocation to every single ActiveRecord instance of a model with at least 1 associated object.

@natematykiewicz
Copy link
Contributor Author

Oh, you'd initialize it to nil. I figured you'd initialize it to an empty Hash. Yeah, that seems fine.

natematykiewicz and others added 3 commits January 7, 2024 14:18
Co-authored-by: Kasper Timm Hansen <hey@kaspth.com>
@natematykiewicz
Copy link
Contributor Author

@kaspth it should be good to go now

@natematykiewicz
Copy link
Contributor Author

Oh, the documentation needs to change too I guess

@kaspth
Copy link
Owner

kaspth commented Jan 7, 2024

I'll fix up the documentation after merging.

@kaspth kaspth merged commit c32707e into kaspth:main Jan 7, 2024
2 checks passed
kaspth added a commit that referenced this pull request Jan 7, 2024
@natematykiewicz natematykiewicz deleted the object_shape_friendliness branch January 9, 2024 18:22
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.

None yet

2 participants