Perf: avoid dupes add fallback logic for coders #12188
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
NOTE: please hold off on merging this until I fully test it and run it for a bit. Comments totally welcome :) |
| } | ||
| } | ||
| column = @columns_hash[name] if @columns_hash | ||
| column = self.class.column_types[name] unless column |
There was a problem hiding this comment.
column ||= self.class.column_types[name]
There was a problem hiding this comment.
oops :) will fix
|
I just run the discourse bench on this and got the following: Before After Its a noticeable improvement, brings Discourse home page only a smidgen slower than it was on rails 3 but the topic page is now much faster. For comparison, these are the numbers on Rails 3 |
|
I think this is good to merge, thoughts? |
|
@SamSaffron thanks! |
tenderlove
added a commit
that referenced
this pull request
Sep 11, 2013
Perf: avoid dupes add fallback logic for coders
tenderlove
added a commit
that referenced
this pull request
Sep 11, 2013
Perf: avoid dupes add fallback logic for coders
|
|
||
| @attributes_cache = {} | ||
| @column_types = self.class.column_types | ||
| @column_types_override = fresh_object.instance_variable_get('@columns_types_override') |
There was a problem hiding this comment.
There was a typo on the instance var call here, I fixed that on master and 4-0, lets keep an eye on travis now ;)
There was a problem hiding this comment.
sorry, totally my bad
There was a problem hiding this comment.
I think I know why that happened ... columns_hash vs. column_types we should make that consistent at least.
column_types_hash and. column_types perhaps, maybe figure out a way of eliminating @columns_hash?
|
@tenderlove @SamSaffron I've been using 4-0-stable happily until this was merged, 918a148 is the first 'bad' commit which causes this issue: There are four migrations that should have run but it gets to the end of the first one and fails to continue. Not sure why, I'll go back to using the ref before this started failing for now. Thanks! ❤️ |
|
@parndt is there any chance you can mail me details to a repro at sam.saffron@gmail.com It appears that
|
|
@SamSaffron here you go https://github.com/parndt/rails-pull-12188 Just clone, bundle, rake db:migrate, kaboom! |
|
@SamSaffron wow thanks for the quick turnaround |
|
@parndt make sure you are using latest 4-0-stable and protected_attributes master and you issue will be fixed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This had a pretty significant perf improvement, will run numbers tomorrow and add to the ticket, it changes semantics of @column_hash , nonetheless all the tests seem to pass.
It avoids fairly significant repetitive copies of columns in a table on each instance. If this is found to be safe its a strong candidate for backport into 4.0.