Replacing plus with push in array

[rishav-csenitjsr]

Summary

Every time an object is getting added using "+" and is being assigned to itself. The new object id is created. Instead, we should use a "push" in which a new object id is not created.
Sample code to explain this:
:001 > valid = [:counter_cache, :join_table, :index_errors]
=> [:counter_cache, :join_table, :index_errors]
002 > valid.object_id
=> 70353367984560
003 > valid += [:as, :foreign_type]
=> [:counter_cache, :join_table, :index_errors, :as, :foreign_type]
004 > valid.object_id
=> 70353359749500
005 > valid.push(:through, :source, :source_type)
=> [:counter_cache, :join_table, :index_errors, :as, :foreign_type, :through, :source, :source_type]
006 > valid.object_id
=> 70353359749500

Other Information

[kaspth]

We're now mutating the return value from super, are we sure that's safe? How often is valid_options called, I think only on definition time? If so, given there's an upper bound on the number of associations definitions in a Rails app, does this matter? Are the intermediate Arrays retained or GC'ed too? We don't like to micro-optimize without reasons such as these already given, so please give some thought to these questions. Thanks!

(Note for future reports: creating a new object_id isn't troublesome and what we'd fix, it's that it signifies a new array gets created and therefore allocates more memory)

[rishav-csenitjsr]

We're now mutating the return value from super, are we sure that's safe? How often is valid_options called, I think only on definition time? If so, given there's an upper bound on the number of associations definitions in a Rails app, does this matter? Are the intermediate Arrays retained or GC'ed too? We don't like to micro-optimize without reasons such as these already given, so please give some thought to these questions. Thanks!

(Note for future reports: creating a new object_id isn't troublesome and what we'd fix, it's that it signifies a new array gets created and therefore allocates more memory)

Hi @kaspth , I have kept the first line as it is. I have done benchmarking of the memory allocation and I found out there is a significant difference in memory allocation as you can see from the below:
`irb(main):020:0> def benchmarking_method
irb(main):021:1> valid = []
irb(main):022:1> valid.push(:counter_cache, :join_table, :index_errors)
irb(main):023:1> valid.push(:as, :foreign_type)
irb(main):024:1> valid.push(:through, :source, :source_type)
irb(main):025:1> valid
irb(main):026:1> end
=> :benchmarking_method
irb(main):027:0>
irb(main):028:0>
irb(main):029:0> def benchmarking_method_old
irb(main):030:1> valid = [:counter_cache, :join_table, :index_errors]
irb(main):031:1> valid += [:as, :foreign_type]
irb(main):032:1> valid += [:through, :source, :source_type]
irb(main):033:1> valid
irb(main):034:1> end
=> :benchmarking_method_old
irb(main):035:0> stats_old = AllocationStats.trace {benchmarking_method_old}
=> [#<AllocationStats::Allocation:0x00005559617a0930 @object=[:counter_cache, :join_table, :index_errors, :as, :foreign_type, :through, :source, :source_type], @memsize=104, @sourcefile="(irb)", @sourceLine=32, @class_path="Object", @method_id=:benchmarking_method_old>, #<AllocationStats::Allocation:0x00005559617a0778 @object=[:through, :source, :source_type], @memsize=40, @sourcefile="(irb)", @sourceLine=32, @class_path="Object", @method_id=:benchmarking_method_old>, #<AllocationStats::Allocation:0x00005559617b3d28 @object=[:counter_cache, :join_table, :index_errors, :as, :foreign_type], @memsize=80, @sourcefile="(irb)", @sourceLine=31, @class_path="Object", @method_id=:benchmarking_method_old>, #<AllocationStats::Allocation:0x00005559617b39e0 @object=[:as, :foreign_type], @memsize=40, @sourcefile="(irb)", @sourceLine=31, @class_path="Object", @method_id=:benchmarking_method_old>, #<AllocationStats::Allocation:0x00005559617b3760 @object=[:counter_cache, :join_table, :index_errors], @memsize=40, @sourcefile="(irb)", @sourceLine=30, @class_path="Object", @method_id=:benchmarking_method_old>]
irb(main):036:0> stats = AllocationStats.trace {benchmarking_method}
=> [#<AllocationStats::Allocation:0x00005559616cc770 @object=[:counter_cache, :join_table, :index_errors, :as, :foreign_type, :through, :source, :source_type], @memsize=208, @sourcefile="(irb)", @sourceLine=21, @class_path="Object", @method_id=:benchmarking_method>]
irb(main):037:0> puts stats_old.allocations(alias_paths: true).to_text
sourcefile sourceline class_path method_id memsize class

---

(irb) 32 Object benchmarking_method_old 104 Array
(irb) 32 Object benchmarking_method_old 40 Array
(irb) 31 Object benchmarking_method_old 80 Array
(irb) 31 Object benchmarking_method_old 40 Array
(irb) 30 Object benchmarking_method_old 40 Array
=> nil
irb(main):038:0> puts stats.allocations(alias_paths: true).to_text
sourcefile sourceline class_path method_id memsize class

---

(irb) 21 Object benchmarking_method 208 Array
=> nil`
I have used allocation_stats gem.

[kaspth]

Yes, there’s fewer Array allocations, that doesn’t mean I want to set a precedent where we start microoptimizing without knowing the context. Please address my other questions 😊

I don’t like that we’ve just swapped the super line back to + just like that, feels like we don’t know what we’re doing here.

[rishav-csenitjsr]

I think it doesn’t matter much as the upper bound is fixed but I think sometime later we should micro optimise each file for providing best performance. What’s your suggestion @kaspth ?

[kaspth]

Absolutely not. We generally optimize for readability because that’s way more important most of the time to such a large codebase. If what we “optimize” has 0.000001 impact on an actual app, it’s been a waste of everybody’s time.

Sorry, I don’t think it’s worth continuing this. There’s too much need for conversation for a relatively questionable gain. Appreciate the attempt 🙏