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

[Immutablize#convert_value] reduce number of class comparisons #14234

Merged
merged 1 commit into from Mar 28, 2024

Conversation

dafyddcrosby
Copy link
Contributor

Description

Noticed while doing #14226 (and orthogonal to it): on large amounts of Immutablize calls a decent amount of time is spent simply doing class comparisons for non-Hash/Array objects. We can reduce the comparisons by nearly half while retaining the prior optimization by checking for ImmutableArray/ImmutableMash after the object has already been identified as a Hash or an Array.

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • If Gemfile.lock has changed, I have used --conservative to do it and included the full output in the Description above.
  • All new and existing tests passed.
  • All commits have been signed-off for the Developer Certificate of Origin.

@dafyddcrosby dafyddcrosby requested review from a team as code owners February 9, 2024 23:32
Copy link

sonarcloud bot commented Feb 9, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

@tpowell-progress tpowell-progress left a comment

Choose a reason for hiding this comment

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

Requesting data on calls and timing

@tpowell-progress
Copy link
Contributor

M1 MacPro with Ruby 3.1.2

Rehearsal ---------------------------------------------------------------------------
convert_value_pre13733: empty hashes	     0.050371   0.002531   0.052902 (  0.053425)
convert_value_pre13733: empty arrays	     0.036591   0.000821   0.037412 (  0.038140)
convert_value_pre13733: N str arrays	     0.321194   0.001231   0.322425 (  0.326191)
convert_value_pre13733: 2xN str arrays	   0.638575   0.001589   0.640164 (  0.646488)
convert_value_pre13733: AwfulNest	        1.687074   0.007897   1.694971 (  1.715149)
convert_value_pre13733: 2xAwfulNest	      3.438798   0.015080   3.453878 (  3.495792)
------------------------------------------------------------------ total: 6.201752sec

                                              user     system      total        real
convert_value_pre13733: empty hashes	     0.034488   0.000411   0.034899 (  0.035250)
convert_value_pre13733: empty arrays	     0.034777   0.000366   0.035143 (  0.035551)
convert_value_pre13733: N str arrays	     0.313847   0.000625   0.314472 (  0.319183)
convert_value_pre13733: 2xN str arrays	   0.620730   0.001166   0.621896 (  0.630116)
convert_value_pre13733: AwfulNest	        1.684152   0.006627   1.690779 (  1.709477)
convert_value_pre13733: 2xAwfulNest	      3.418117   0.012961   3.431078 (  3.471830)

Rehearsal -----------------------------------------------------------------------
convert_value_main: empty hashes	     0.036842   0.000603   0.037445 (  0.037930)
convert_value_main: empty arrays	     0.036118   0.000251   0.036369 (  0.036883)
convert_value_main: N str arrays	     0.321115   0.000740   0.321855 (  0.325192)
convert_value_main: 2xN str arrays	   0.638158   0.001536   0.639694 (  0.644187)
convert_value_main: AwfulNest	        1.829451   0.007348   1.836799 (  1.851214)
convert_value_main: 2xAwfulNest	      1.992518   0.006484   1.999002 (  2.019828)
-------------------------------------------------------------- total: 4.871164sec

                                          user     system      total        real
convert_value_main: empty hashes	     0.035116   0.000602   0.035718 (  0.036236)
convert_value_main: empty arrays	     0.034935   0.000357   0.035292 (  0.035672)
convert_value_main: N str arrays	     0.310822   0.000535   0.311357 (  0.314946)
convert_value_main: 2xN str arrays	   0.622768   0.001208   0.623976 (  0.628791)
convert_value_main: AwfulNest	        1.814281   0.006675   1.820956 (  1.841205)
convert_value_main: 2xAwfulNest	      1.988011   0.006461   1.994472 (  2.020952)

Rehearsal ----------------------------------------------------------------------------------
convert_value_classcomparison: empty hashes	     0.035557   0.000470   0.036027 (  0.036541)
convert_value_classcomparison: empty arrays	     0.034544   0.000196   0.034740 (  0.035242)
convert_value_classcomparison: N str arrays	     0.272643   0.000617   0.273260 (  0.276573)
convert_value_classcomparison: 2xN str arrays	   0.535337   0.001092   0.536429 (  0.543886)
convert_value_classcomparison: AwfulNest	        1.722579   0.007491   1.730070 (  1.749565)
convert_value_classcomparison: 2xAwfulNest	      1.933518   0.007164   1.940682 (  1.963775)
------------------------------------------------------------------------- total: 4.551208sec

                                                     user     system      total        real
convert_value_classcomparison: empty hashes	     0.034731   0.000567   0.035298 (  0.035467)
convert_value_classcomparison: empty arrays	     0.034082   0.000684   0.034766 (  0.035111)
convert_value_classcomparison: N str arrays	     0.268007   0.000541   0.268548 (  0.271741)
convert_value_classcomparison: 2xN str arrays	   0.533411   0.001138   0.534549 (  0.539218)
convert_value_classcomparison: AwfulNest	        1.713323   0.006515   1.719838 (  1.739308)
convert_value_classcomparison: 2xAwfulNest	      1.921875   0.006387   1.928262 (  1.950469)

Rehearsal ------------------------------------------------------------------------------
convert_value_frozencheck: empty hashes	     0.037947   0.000405   0.038352 (  0.038896)
convert_value_frozencheck: empty arrays	     0.034877   0.000181   0.035058 (  0.035437)
convert_value_frozencheck: N str arrays	     0.343371   0.000990   0.344361 (  0.351555)
convert_value_frozencheck: 2xN str arrays	   0.558948   0.000970   0.559918 (  0.567667)
convert_value_frozencheck: AwfulNest	        1.864030   0.007681   1.871711 (  1.892340)
convert_value_frozencheck: 2xAwfulNest	      2.051410   0.007721   2.059131 (  2.104084)
--------------------------------------------------------------------- total: 4.908531sec

                                                 user     system      total        real
convert_value_frozencheck: empty hashes	     0.037116   0.000880   0.037996 (  0.039014)
convert_value_frozencheck: empty arrays	     0.033435   0.000344   0.033779 (  0.033979)
convert_value_frozencheck: N str arrays	     0.329345   0.000563   0.329908 (  0.334286)
convert_value_frozencheck: 2xN str arrays	   0.546280   0.000750   0.547030 (  0.553623)
convert_value_frozencheck: AwfulNest	        1.783799   0.005227   1.789026 (  1.805012)
convert_value_frozencheck: 2xAwfulNest	      1.948865   0.005102   1.953967 (  1.973575)

Rehearsal -----------------------------------------------------------------------
convert_value_both: empty hashes	     0.036070   0.000455   0.036525 (  0.036716)
convert_value_both: empty arrays	     0.037475   0.000239   0.037714 (  0.038098)
convert_value_both: N str arrays	     0.291174   0.000695   0.291869 (  0.295277)
convert_value_both: 2xN str arrays	   0.454173   0.000892   0.455065 (  0.459213)
convert_value_both: AwfulNest	        1.678815   0.004975   1.683790 (  1.697143)
convert_value_both: 2xAwfulNest	      1.894839   0.004951   1.899790 (  1.918278)
-------------------------------------------------------------- total: 4.404753sec

                                          user     system      total        real
convert_value_both: empty hashes	     0.035149   0.000449   0.035598 (  0.035968)
convert_value_both: empty arrays	     0.033942   0.000364   0.034306 (  0.034578)
convert_value_both: N str arrays	     0.283407   0.000538   0.283945 (  0.287866)
convert_value_both: 2xN str arrays	   0.445680   0.000795   0.446475 (  0.452628)
convert_value_both: AwfulNest	        1.704889   0.005942   1.710831 (  1.730264)
convert_value_both: 2xAwfulNest	      1.892239   0.005574   1.897813 (  1.919692)

@dafyddcrosby
Copy link
Contributor Author

Benchmarked on my personal laptop: https://gist.github.com/dafyddcrosby/00e8b526093b4e2819de48de30b088f5

I'm open to other ways of comparing the methods, but so far this does appear to be faster on multiple platforms and architectures

Signed-off-by: David Crosby <dcrosby@fb.com>
Copy link

sonarcloud bot commented Mar 26, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@tpowell-progress
Copy link
Contributor

@dafyddcrosby waiting on Verify just to be safe.

@tpowell-progress tpowell-progress merged commit 8b41b41 into chef:main Mar 28, 2024
54 checks passed
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.

None yet

3 participants