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

PERF: 20% faster pk attribute access #36052

Merged
merged 1 commit into from Apr 22, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -465,7 +465,7 @@ def readonly_attribute?(name)
end

def pk_attribute?(name)
name == self.class.primary_key
name == @primary_key
end
end
end
@@ -16,34 +16,32 @@ def to_key

# Returns the primary key column's value.
def id
primary_key = self.class.primary_key
_read_attribute(primary_key) if primary_key
_read_attribute(@primary_key)

This comment has been minimized.

Copy link
@kamipo

kamipo Apr 22, 2019

Author Member

This if primary_key is redundant, since _read_attribute(nil) returns nil.

end

# Sets the primary key column's value.
def id=(value)
primary_key = self.class.primary_key
_write_attribute(primary_key, value) if primary_key
_write_attribute(@primary_key, value)

This comment has been minimized.

Copy link
@kamipo

kamipo Apr 22, 2019

Author Member

Previously #id= behaves as no-op if primary key doesn't exist.
But I think that calling #id= on no-pk object is already something wrong, raising MissingAttributeError is preferable than ignore any error to me.

And also, since most models has pk, this guard is almost redundant.
E.g. persistence.rb#L932 checks if @primary_key three times as before, now only once.

new_id = self.class._insert_record(
attributes_with_values(attribute_names)
)
self.id ||= new_id if @primary_key

end

# Queries the primary key column's value.
def id?
query_attribute(self.class.primary_key)
query_attribute(@primary_key)
end

# Returns the primary key column's value before type cast.
def id_before_type_cast
read_attribute_before_type_cast(self.class.primary_key)
read_attribute_before_type_cast(@primary_key)
end

# Returns the primary key column's previous value.
def id_was
attribute_was(self.class.primary_key)
attribute_was(@primary_key)
end

# Returns the primary key column's value from the database.
def id_in_database
attribute_in_database(self.class.primary_key)
attribute_in_database(@primary_key)
end

private
@@ -116,7 +114,7 @@ def get_primary_key(base_name) #:nodoc:
#
# Project.primary_key # => "foo_id"
def primary_key=(value)
@primary_key = value && value.to_s
@primary_key = value && -value.to_s

This comment has been minimized.

Copy link
@DamirSvrtan

DamirSvrtan Apr 22, 2019

What does the minus do here?

This comment has been minimized.

Copy link
@kamipo

kamipo Apr 22, 2019

Author Member

Ruby 2.5 and higher has a string deduplication (fstring table), String#-@ registers the string to fstring table and fetches the deduplicated instance.
The most @primary_keys are "id", the deduplication will reduce memory usage.

@quoted_primary_key = nil
@attributes_builder = nil
end
@@ -31,8 +31,7 @@ def read_attribute(attr_name, &block)
name = self.class.attribute_alias(name)
end

primary_key = self.class.primary_key
name = primary_key if name == "id" && primary_key
name = @primary_key if name == "id" && @primary_key
_read_attribute(name, &block)
end

@@ -35,8 +35,7 @@ def write_attribute(attr_name, value)
name = self.class.attribute_alias(name)
end

primary_key = self.class.primary_key
name = primary_key if name == "id" && primary_key
name = @primary_key if name == "id" && @primary_key
_write_attribute(name, value)
end

@@ -174,8 +174,7 @@ def find(*ids) # :nodoc:

record = statement.execute([id], connection)&.first
unless record
raise RecordNotFound.new("Couldn't find #{name} with '#{primary_key}'=#{id}",
name, primary_key, id)
raise RecordNotFound.new("Couldn't find #{name} with '#{key}'=#{id}", name, key, id)
end
record
end
@@ -398,7 +397,7 @@ def init_with_attributes(attributes, new_record = false) # :nodoc:
##
def initialize_dup(other) # :nodoc:
@attributes = @attributes.deep_dup
@attributes.reset(self.class.primary_key)
@attributes.reset(@primary_key)

_run_initialize_callbacks

@@ -570,6 +569,7 @@ def to_ary
end

def init_internals
@primary_key = self.class.primary_key
@readonly = false
@destroyed = false
@marked_for_destruction = false
@@ -87,7 +87,7 @@ def _update_row(attribute_names, attempted_action = "update")

affected_rows = self.class._update_record(
attributes_with_values(attribute_names),
self.class.primary_key => id_in_database,
@primary_key => id_in_database,
locking_column => previous_lock_value
)

@@ -110,7 +110,7 @@ def destroy_row
locking_column = self.class.locking_column

affected_rows = self.class._delete_record(
self.class.primary_key => id_in_database,
@primary_key => id_in_database,
locking_column => read_attribute_before_type_cast(locking_column)
)

@@ -353,6 +353,7 @@ def delete(id_or_array)
end

def _insert_record(values) # :nodoc:
primary_key = self.primary_key
primary_key_value = nil

if primary_key && Hash === values
@@ -674,7 +675,7 @@ def update_columns(attributes)

affected_rows = self.class._update_record(
attributes,
self.class.primary_key => id_in_database
@primary_key => id_in_database
)

affected_rows == 1
@@ -874,7 +875,7 @@ def destroy_row
end

def _delete_row
self.class._delete_record(self.class.primary_key => id_in_database)
self.class._delete_record(@primary_key => id_in_database)
end

def _touch_row(attribute_names, time)
@@ -890,7 +891,7 @@ def _touch_row(attribute_names, time)
def _update_row(attribute_names, attempted_action = "update")
self.class._update_record(
attributes_with_values(attribute_names),
self.class.primary_key => id_in_database
@primary_key => id_in_database
)
end

@@ -928,7 +929,7 @@ def _create_record(attribute_names = self.attribute_names)
attributes_with_values(attribute_names)
)

self.id ||= new_id if self.class.primary_key
self.id ||= new_id if @primary_key

@new_record = false

@@ -432,9 +432,8 @@ def restore_transaction_record_state(force_restore_state = false)
end
@mutations_from_database = nil
@mutations_before_last_save = nil
pk = self.class.primary_key
if pk && @attributes.fetch_value(pk) != restore_state[:id]
@attributes.write_from_user(pk, restore_state[:id])
if @attributes.fetch_value(@primary_key) != restore_state[:id]
@attributes.write_from_user(@primary_key, restore_state[:id])
end
freeze if restore_state[:frozen?]
end
@@ -203,6 +203,14 @@ def test_create_without_primary_key_no_extra_query
assert_queries(3, ignore_none: true) { klass.create! }
end

def test_assign_id_raises_error_if_primary_key_doesnt_exist
klass = Class.new(ActiveRecord::Base) do
self.table_name = "dashboards"
end
dashboard = klass.new
assert_raises(ActiveModel::MissingAttributeError) { dashboard.id = "1" }
end

if current_adapter?(:PostgreSQLAdapter)
def test_serial_with_quoted_sequence_name
column = MixedCaseMonkey.columns_hash[MixedCaseMonkey.primary_key]
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.