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

Use a generic getter with __callee__ #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

headius
Copy link
Contributor

@headius headius commented Aug 9, 2023

This modifies the getter definition to use a single method that uses __callee__ to access the hash.

def __get_callee__!
  @table[__callee__]
end

On versions of Ruby 2.0+ and JRuby 9.4.2+ __callee__ will return the aliased name, which in this case is the key for the table.

This eliminates one of the Proc-based methods per field and also performs better on CRuby:

Before:

Warming up --------------------------------------
                 get    44.000  i/100ms
                 set    46.000  i/100ms
Calculating -------------------------------------
                 get    504.731  (± 8.5%) i/s -      2.508k in   5.024439s
                 set    506.779  (± 1.4%) i/s -      2.576k in   5.084061s

After:

Warming up --------------------------------------
                 get    80.000  i/100ms
                 set    65.000  i/100ms
Calculating -------------------------------------
                 get    808.319  (± 0.7%) i/s -      4.080k in   5.047805s
                 set    662.678  (± 0.9%) i/s -      3.315k in   5.002857s

It also uses less memory; creating 100k OpenStruct in the same way as the benchmark uses 205.2MB before and 164MB after.

Note that a similar memory savings is possible by using the following setter:

def __set_callee__!(value)
  @name[__callee__[0..-2].intern] = value
end

But it is quite a bit slower than the Proc version. I think it would be worth exploring other ways we can simplify the setters too.

This modifies the getter definition to use a single method that
uses `__callee__` to access the hash.

def __get_callee__!
  @table[__callee__]
end

On versions of Ruby 2.0+ and JRuby 9.4.2+ __callee__ will return
the aliased name, which in this case is the key for the table.

This eliminates one of the Proc-based methods per field and also
performs better on CRuby:

Before:

Warming up --------------------------------------
                 get    44.000  i/100ms
                 set    46.000  i/100ms
Calculating -------------------------------------
                 get    504.731  (± 8.5%) i/s -      2.508k in   5.024439s
                 set    506.779  (± 1.4%) i/s -      2.576k in   5.084061s

After:

Warming up --------------------------------------
                 get    80.000  i/100ms
                 set    65.000  i/100ms
Calculating -------------------------------------
                 get    808.319  (± 0.7%) i/s -      4.080k in   5.047805s
                 set    662.678  (± 0.9%) i/s -      3.315k in   5.002857s

It also uses less memory; creating 100k OpenStruct in the same
way as the benchmark uses 205.2MB before and 164MB after.
@headius
Copy link
Contributor Author

headius commented Aug 9, 2023

Relates to jruby/jruby#7876 where we noticed ostruct uses a LOT more memory now that it pre-creates all singleton methods in response to the hash form of OpenStruct#new.

@headius
Copy link
Contributor Author

headius commented Aug 9, 2023

Note: I also tried using class_eval to create these methods with literal symbols. That was easily the fastest-performing mechanism for getter/setter methods, but it also consumed even more memory than the Proc version. 🤷‍♂️

@headius headius requested review from marcandre and nobu August 9, 2023 18:16
@headius
Copy link
Contributor Author

headius commented Aug 9, 2023

FWIW I know this library is not intended for performance-sensitive applications, but I think the speed and memory improvements here are worth the small change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant