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.

Sign up for GitHub

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

Jump to bottom

Fix multiple inheritance-related bugs #241

Merged
merged 2 commits into from
Dec 9, 2021
Merged

Fix multiple inheritance-related bugs #241

merged 2 commits into from
Dec 9, 2021

Conversation

JacobEvelyn
Copy link
Member

This commit fixes several bugs related to inheritance by
delaying the calculation of the index used to store memoized
method results until the method itself is called. This incurs
a slight performance penalty the first time a method is called.

Closes #238
Closes #239

Before merging:

  • Copy the table printed at the end of the latest benchmark results into the README.md and update this PR
  • If this change merits an update to CHANGELOG.md, add an entry following Keep a Changelog guidelines with semantic versioning

@codecov
Copy link

codecov bot commented Dec 6, 2021 •
edited
Loading

Codecov Report

Merging #241 (761d8aa) into main (a97cd40) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #241   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          201       206    +5     
=========================================
+ Hits           201       206    +5
Impacted Files Coverage Δ
lib/memo_wise.rb 100.00% <100.00%> (ø)
lib/memo_wise/internal_api.rb 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a97cd40...761d8aa. Read the comment docs.

@JacobEvelyn JacobEvelyn force-pushed the multiple-inheritance-lazy-eval branch 3 times, most recently from 270e62b to 4dda71a Compare December 6, 2021 21:15
PikachuEXE
PikachuEXE reviewed Dec 7, 2021
View reviewed changes
Copy link
Contributor

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion on CHANGELOG

I can't review the code changes anyway

Well I guess I can run benchmark

CHANGELOG.md Outdated Show resolved Hide resolved
@PikachuEXE
Copy link
Contributor

Benchmark results

`3.0.3`

ruby 3.0.3p157 (2021-11-24 revision 3fb7d2cadc) [arm64-darwin21]

Method arguments Dry::Core (0.7.1) Memery (1.4.0)
() (none) 1.35x 12.60x
(a) 1.74x 7.96x
(a, b) 0.49x 2.06x
(a:) 1.74x 15.41x
(a:, b:) 0.52x 4.33x
(a, b:) 0.50x 4.11x
(a, *args) 0.91x 1.96x
(a:, **kwargs) 0.85x 3.07x
(a, *args, b:, **kwargs) 0.64x 1.66x
`2.7.5`

ruby 2.7.5p203 (2021-11-24 revision f69aeb8314) [arm64-darwin21]

Method arguments DDMemoize (1.0.0) Memoist (0.16.2) Memoized (1.0.2) Memoizer (1.0.3)
() (none) 26.18x 2.93x 1.35x 3.54x
(a) 17.74x 12.42x 9.39x 10.38x
(a, b) 3.04x 2.29x 1.82x 1.98x
(a:) 24.15x 19.65x 16.77x 17.67x
(a:, b:) 4.69x 3.89x 3.39x 3.55x
(a, b:) 4.33x 3.58x 3.13x 3.28x
(a, *args) 2.94x 2.23x 1.93x 1.95x
(a:, **kwargs) 2.60x 2.19x 1.92x 2.02x
(a, *args, b:, **kwargs) 1.99x 1.69x 1.58x 1.61x

@JacobEvelyn JacobEvelyn mentioned this pull request Dec 7, 2021
Closed
@JacobEvelyn JacobEvelyn force-pushed the multiple-inheritance-lazy-eval branch 6 times, most recently from 9412bc9 to 5b09d6e Compare December 7, 2021 19:59
JacobEvelyn
JacobEvelyn commented Dec 7, 2021
View reviewed changes
spec/support/shared_context_for_module_methods_via_class_scope.rb
@@ -1,10 +1,8 @@
# frozen_string_literal: true

RSpec.shared_context "with context for module methods via scope 'class << self'" do
# NOTE: This use of `before(:all)` is a performance optimization that shaves
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This no longer seemed necessary; our tests run in seconds in all Ruby versions as far as I can tell.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer these in their own separate PR, is that ok?

JacobEvelyn
JacobEvelyn commented Dec 7, 2021
View reviewed changes
spec/support/shared_context_for_class_methods_via_class_scope.rb
@@ -1,10 +1,8 @@
# frozen_string_literal: true

RSpec.shared_context "with context for class methods via scope 'class << self'" do
# NOTE: This use of `before(:all)` is a performance optimization that shaves
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This no longer seemed necessary; our tests run in seconds in all Ruby versions as far as I can tell.

JacobEvelyn and others added 2 commits December 9, 2021 15:56
@JacobEvelyn @ms-ati
Fix multiple inheritance-related bugs
d464b18 
This commit fixes several bugs related to inheritance by
delaying the calculation of the index used to store memoized
method results until the method itself is called. This incurs
a slight performance penalty the first time a method is called.

Closes #238
Closes #239
@ms-ati
PR: test for race condition
761d8aa
@ms-ati ms-ati force-pushed the multiple-inheritance-lazy-eval branch from 504d69d to 761d8aa Compare December 9, 2021 21:00
ms-ati
ms-ati approved these changes Dec 9, 2021
View reviewed changes
Copy link
Contributor

@ms-ati ms-ati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JacobEvelyn I'm comfortable that the module_eval approach doesn't seem to have race condition problems given the thread_safety_spec.rb change to create a new class each loop iteration, meaning we are racing two threads to rewrite the method

@JacobEvelyn JacobEvelyn merged commit b20b2ca into main Dec 9, 2021
@JacobEvelyn JacobEvelyn deleted the multiple-inheritance-lazy-eval branch December 9, 2021 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PikachuEXE PikachuEXE PikachuEXE left review comments

@ms-ati ms-ati ms-ati approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error memo_wising in subclass with included module Multiple composition inheritance bug
3 participants
@JacobEvelyn @PikachuEXE @ms-ati