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

Dynamic attribute optimizations #281

Merged
merged 5 commits into from
Apr 13, 2018
Merged

Dynamic attribute optimizations #281

merged 5 commits into from
Apr 13, 2018

Conversation

danbernier
Copy link
Contributor

I was profiling some code that handles json_api_client results, and makes a lot of use of the dynamic attribute methods. I noticed that if I just implemented my own getter methods:

class Article
  def title
    attributes[:title]
  end
end

...then it got surprisingly faster.

I looked at the source, and spotted a few optimizations I could make. Here are 4, each with before/after benchmark times (the benchmark code is committed here as a unit test, which we can yank or move before this is merged).

The only commit without benchmark times is the change to use end_with?('=') instead of =~ /^(.*)=$/, because it didn't actually speed it up. I committed it here as a curiosity; because I think ends_with?('=') is clearer than the regex; and because I've seen people say that it's faster, so this heads off the conversation.

Probably the most controversial optimization is the last one: to auto-define getter methods for attributes from DynamicAttributes#method_missing. This is a significant speed-up: basically, we'll never have to run that method_missing logic again for that method name. Since the classes already respond to those methods, and handle those messages in an equivalent (but slower) way, this seems OK to me. Also, I don't think we have to worry about overwriting client code, because by definition, if #method_missing is running, then they didn't define this one.

Here's the overall benchmark difference, from the first commit (which only adds the benchmark unit test), to the last (with the 4 optimizations):

Before:

       user     system      total        real
read:   1.090000   0.010000   1.100000 (  1.091464)
write:  1.130000   0.000000   1.130000 (  1.136750)

After:

       user     system      total        real
read:   0.020000   0.000000   0.020000 (  0.016366)
write:  0.690000   0.000000   0.690000 (  0.687914)

This obviously is the wrong place for this, but it lets me run things
quickly. It'll come out before the end.
Sample benchmark before:

    user     system      total        real
    read:   1.070000   0.000000   1.070000 (  1.077592)
    write:  1.130000   0.010000   1.140000 (  1.142294)

Sample benchmark after:

    user     system      total        real
    read:   0.580000   0.010000   0.590000 (  0.593090)
    write:  1.170000   0.000000   1.170000 (  1.180000)
This avoids repeatedly checking whether the class responds to
`#key_formatter`, calling `#key_formatter`, and checking whether its
result is nil.

Sample before benchmark:

       user     system      total        real
       read:   0.580000   0.000000   0.580000 (  0.576614)
       write:  1.130000   0.010000   1.140000 (  1.141003)

Sample after benchmark:

       user     system      total        real
       read:   0.570000   0.000000   0.570000 (  0.571892)
       write:  0.670000   0.010000   0.680000 (  0.672004)
Not sure this matters yet.
Sample before benchmark:

       user     system      total        real
       read:   0.570000   0.000000   0.570000 (  0.576622)
       write:  0.670000   0.000000   0.670000 (  0.672334)

Sample after benchmark:

       user     system      total        real
       read:   0.020000   0.000000   0.020000 (  0.013004)
       write:  0.640000   0.000000   0.640000 (  0.647726)

Because this defines a method for an attribute when the attribute is
accessed, I had to change the spec that checked for handling missing
methods for attributes, because it was accessing the attribute, and
therefore defining the method.
@jsmestad
Copy link
Collaborator

@danbernier great work on this! I've pulled this into a released fork of this gem jsonapi-consumer along with some other outstanding bug fixes.

@chingor13 chingor13 merged commit e5325d1 into JsonApiClient:master Apr 13, 2018
@danbernier danbernier deleted the dynamic-attribute-optimizations branch April 16, 2018 13:52
@danbernier
Copy link
Contributor Author

@chingor13, thanks for merging this! Glad to see this project is still alive.

And @jsmestad thank you, too, glad you found it helpful!

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.

3 participants