Introduce an Attribute object to handle the type casting dance

[sgrif]

Depends on #15429 and includes the changes from that PR as well. There's a lot more that can be moved to these, but this felt like a good place to introduce the object. Plans are:

• Remove all knowledge of type casting from the columns, beyond a
  reference to the cast_type
• Move type_cast_for_database to these objects
• Potentially make them mutable, introduce a state machine, and have
  dirty checking handled here as well
• Move attribute, decorate_attribute, and anything else that
  modifies types to mess with this object, not the columns hash
• Introduce a collection object to manage these, and reduce allocations

[egilburg]

pretty heavy logic here. could it be extracted to private method like

@attributes = attributes_from_raw_column_defaults

[sgrif]

Agreed, not happy with defaults right now, planning much more work on those specifically. Needed to cut the PR at some point. Would rather this stay as close to the original structure as possible for the time being.

[thedarkone]

Do I read it correctly that this will now allocate an Attribute object for every attribute of an AR object?

[sgrif]

And allocate one less string per attribute, one less hash per Base object,
and go through fewer branches on both read and write. Yes.
On Jun 10, 2014 5:22 AM, "thedarkone" notifications@github.com wrote:

Do I read it correctly that this will now allocate an Attribute object
for every attribute of an AR object?

—
Reply to this email directly or view it on GitHub
#15593 (comment).

[thedarkone]

One hash for multiple Attributes objects might work ok (a mapping in a Hash is usually an object on its own).

Where do you save a string?

[sgrif]

The keys of raw_attributes
On Jun 10, 2014 6:03 AM, "thedarkone" notifications@github.com wrote:

One hash for multiple Attributes objects might work ok (a mapping in a
Hash is usually an object on its own).

Where do you save a string?

—
Reply to this email directly or view it on GitHub
#15593 (comment).

[thedarkone]

Damn, I remember a PR that attempted to pre-freeze those (frozen strs don't get duped by Hashes), or was that something else?

[sgrif]

Unsure. This is also a first step, which should enable me to reduce object allocations overall once I start tackling @column_types and @column_types_override.

[egilburg]

Do I read it correctly that this will now allocate an Attribute object for every attribute of an AR object?

AR object or AR class? Instantiating an attribute should happen once per column (or user defined) attribute definition, but not every time an AR object is instantiated and attributes populated, right?

[thedarkone]

AR object or AR class?

AR object (not class).

Instantiating an attribute should happen once per column (or user defined) attribute definition, but not every time an AR object is instantiated and attributes populated, right?

No, it seems to be 1x Attribute.new for each attribute of every AR object instantiated.

[sgrif]

That is correct. However, for each Attribute object we gain, we've cut one string allocation when reading attributes. This will enable us to reduce object allocations overall, as we can reduce the number of caches each instance has to keep (dirty checking will put us over the top)

[egilburg]

Does the attribute hold any instance data? If not, it can be a singleton (and this can be optimized later)

[sgrif]

Yes, it does hold instance data. It keeps the value before and after type casting, and is responsible for managing that.

[rafaelfranca]

Is not better to set @value to nil on the initializer and avoid the call to defined?

[matthewd]

I guess the type_cast could return nil?

[sgrif]

My concern was that type_cast can return falsy values, and we'd like to avoid performing that computation multiple times. That said, I can't imagine any branch that ends with nil or false being that expensive. Any thoughts? Worth a benchmark?

[rafaelfranca]

I think is worth to benchmark. If it is only an optimisation we need to make clear because I imagine if I change it to use ||= tests will not fail and since there is no information about this optimisation it will be lost.

[sgrif]

Benchmark showed defined? is faster when a value is falsy, even when the type does no work but check if it got nil. I do have an explicit test for this case, I'll add a comment as well.

[sgrif]

Updated. Any more feedback?