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

Performance regression with Improve printing of several arguments (#55754) #55893

Closed
Zentrik opened this issue Sep 26, 2024 · 2 comments · Fixed by #55894
Closed

Performance regression with Improve printing of several arguments (#55754) #55893

Zentrik opened this issue Sep 26, 2024 · 2 comments · Fixed by #55894
Labels
performance Must go faster regression Regression in behavior compared to a previous version strings "Strings!"

Comments

@Zentrik
Copy link
Member

Zentrik commented Sep 26, 2024

This pr made the alloc.strings benchmark in BaseBenchmarks 2000% slower in min wall time, use 42.25% more memory and increase allocations by 63.53%.

Also, it worsened compile performance with inference.abstract interpretation.Base.init_stdio(::Ptr{Cvoid}) being 742% slower and inference.allinference.Base.init_stdio(::Ptr{Cvoid}) 15% slower. There is also a slowdown recently in the println(::QuoteNode) benchmark which is probably due to this. (I'm mentioning this as the compile time impact was investigated in the PR).

Until the next nanosoldier report, data available at https://tealquaternion.camdvr.org/compare.html?start=58b239c5b8eaffec3b9b99e6d7c37e8ae6129d6d&end=6e5e87b2cafda840b90347c2e74202d2608d7c29&stat=min-wall-time.

Also, this made the sys.so 11MiB bigger on the buildkite x86_64-linux-gnu build.
Before:

section                      size       addr
.hash                         832        624
.gnu.hash                      60       1456
.dynsym                      2616       1520
.dynstr                      1876       4136
.gnu.version                  218       6012
.gnu.version_r                176       6232
.rela.dyn                 1146744       6408
.rela.plt                    2064    1153152
.init                          36    1159168
.plt                         1392    1159216
.plt.got                       16    1160608
.text                    30754273    1160624
.fini                          14   31914900
.rodata                     92380   31916032
.eh_frame_hdr              375628   32008412
.eh_frame                 1875288   32384040
.ctors                         16   34266496
.dtors                         16   34266512
.dynamic                      480   34266528
.got                          128   34267008
.got.plt                      712   34267136
.data                           8   34267848
.bss                           16   34267856
.lbss                       85872   34267872
.lrodata                   251552   34357840
.ldata                  122156256   34613488
.comment                       17          0
.debug_aranges                128          0
.debug_info              18645878          0
.debug_abbrev               32104          0
.debug_line               9379182          0
.debug_str                 329379          0
.debug_ranges            18087504          0
.debug_gnu_pubnames       1773625          0
.debug_gnu_pubtypes        180702          0
Total                   205177188

After:

/cache/build/builder-amdci5-6/julialang/julia-master/usr/lib/julia/sys.so  :
section                      size       addr
.hash                         832        624
.gnu.hash                      60       1456
.dynsym                      2616       1520
.dynstr                      1876       4136
.gnu.version                  218       6012
.gnu.version_r                176       6232
.rela.dyn                 1179240       6408
.rela.plt                    2064    1185648
.init                          36    1187840
.plt                         1392    1187888
.plt.got                       16    1189280
.text                    30449361    1189296
.fini                          14   31638660
.rodata                     92240   31641600
.eh_frame_hdr              386460   31733840
.eh_frame                 1922456   32120304
.ctors                         16   34049408
.dtors                         16   34049424
.dynamic                      480   34049440
.got                          128   34049920
.got.plt                      712   34050048
.data                           8   34050760
.bss                           16   34050768
.lbss                       84352   34050784
.lrodata                   250816   34139232
.ldata                  134215360   34394144
.comment                       17          0
.debug_aranges                128          0
.debug_info              18946308          0
.debug_abbrev               31998          0
.debug_line               9289599          0
.debug_str                 339061          0
.debug_ranges            17350624          0
.debug_gnu_pubnames       1864018          0
.debug_gnu_pubtypes        189054          0
Total                   216601768
@Zentrik Zentrik added performance Must go faster regression Regression in behavior compared to a previous version strings "Strings!" labels Sep 26, 2024
@Zentrik
Copy link
Member Author

Zentrik commented Sep 27, 2024

Oddly this also seemed to regress the performance of other inference benchmarks like rand(::Float64). I can't replicate locally but doesn't seem to be noise as it persisted for multiple runs.

Significant changes in benchmarks below

Summary

Range Mean Count
Regressions 7.34%, 1019.85% 448.16% 7
Improvements 0.00%, 0.00% 0.00% 0
All 0.00%, 1019.85% 448.16% 7

Benchmarks

Benchmark % Change Significance Factor
inference.abstract interpretation.broadcasting 1019.85% 200.75x
inference.abstract interpretation.rand(Float64) 751.83% 89.52x
inference.abstract interpretation.Base.init_stdio(::Ptr{Cvoid}) 750.08% 22.32x
inference.abstract interpretation.REPL.REPLCompletions.completions 395.36% 7.78x
inference.abstract interpretation.println(::QuoteNode) 200.49% 16.09x
inference.allinference.rand(Float64) 12.17% 1.70x
inference.allinference.broadcasting 7.34% 1.11x

Source: https://tealquaternion.camdvr.org/compare.html?start=220742d6194acba995eda822c82fdf647d6896ee&end=44bef0df7a115334c10abac88aeba333b12cce2d&stat=min-wall-time&nonRelevant=true.

vtjnash added a commit that referenced this issue Sep 27, 2024
Reverts #55754 as it overrode some performance heuristics
which appeared to be giving a significant gain/loss in performance:
Closes #55893
@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 27, 2024

It is probably a cumulative effect of the memory usage from the primary breakage after running those tests in order

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster regression Regression in behavior compared to a previous version strings "Strings!"
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants