Code review comment for ~lucaskanashiro/ubuntu/+source/ruby2.5:bionic-arm64-optimization

### Branch Review ###

* Changelog:
  - [-] old content and logical tag match as expected
  - [√] changelog entry correct version and targeted codename
  - [√] changelog entries correct
  - [√] update-maintainer has been run

* Actual changes:
  - [-] no upstream changes to consider
  - [-] no further upstream version to consider
  - [√] debian changes look safe

* Old Delta:
  - [-] dropped changes are ok to be dropped
  - [-] nothing else to drop
  - [-] changes forwarded upstream/debian (if appropriate)

* New Delta:
  - [-] no new patches added
  - [√] patches match what was proposed upstream
  - [√] patches correctly included in debian/patches/series
  - [x] patches have correct DEP3 metadata
    + Need bug links, see below

* Build/Test:
  - [√] build is ok
  - [√] verified PPA package installs/uninstalls
  - [x] autopkgtest against the PPA package passes
    + see below
  - [-] sanity checks test fine

I didn't attempt to build locally from git-ubuntu, but the PPA seems to have built fine. I did try running the autopkgtest locally using the ppa, but it fails on the run-all test:

autopkgtest [22:24:33]: test rubyconfig: -----------------------]
autopkgtest [22:24:33]: test rubyconfig: - - - - - - - - - - results - - - - - - - - - -
rubyconfig PASS
autopkgtest [22:24:33]: @@@@@@@@@@@@@@@@@@@@ summary
run-all FAIL non-zero exit status 1
bundled-gems PASS
builtin-extensions PASS
rubyconfig PASS

Here's the specific test case output:

FAIL test/rubygems/test_gem_util.rb
-----------------------------------

[/var/lib/gems/2.5.0/specifications/bundler-1.16.1.gemspec] isn't a Gem::Specification (NilClass instead).
[/var/lib/gems/2.5.0/specifications/bundler-audit-1.0.0.gemspec] isn't a Gem::Specification (NilClass instead).
Run options: -v --excludes-dir=test/excludes/ --excludes-dir=debian/tests/excludes/any/ --excludes-dir=debian/tests/excludes/amd64/ --name=!/memory_leak/

# Running tests:

[1/5] TestGemUtil#test_silent_system = 0.05 s
[2/5] TestGemUtil#test_class_popen = 0.04 s
[3/5] TestGemUtil#test_traverse_parents = 0.00 s
[4/5] TestGemUtil#test_traverse_parents_does_not_crash_on_permissions_error = 0.01 s
[5/5] TestGemUtil#test_linked_list_find = 0.00 s

  1) Failure:
TestGemUtil#test_traverse_parents_does_not_crash_on_permissions_error [/tmp/autopkgtest.XIAQZ1/autopkgtest_tmp/test/rubygems/test_gem_util.rb:43]:
--- expected
+++ actual
@@ -1 +1 @@
-"/tmp/test_rubygems_103177/d"
+"/tmp/test_rubygems_103177/d/e/f"

Finished tests in 0.105744s, 47.2839 tests/s, 94.5678 assertions/s.
5 tests, 10 assertions, 1 failures, 0 errors, 0 skips

The DEP3 for the added patches miss their upstream bug links. The arm patch doesn't appear to have an actual bug report afaict but there is a PR with a bit of context:

https://github.com/ruby/ruby/pull/3393
https://bugs.ruby-lang.org/issues/14655

Inline below are some refactoring and typo fixing suggestions.

### SRU Text Review ###

Some suggestions to help improve the SRU text to help it pass.

The timezone patch doesn't appear to have a ubuntu bug. I gather this is added strictly just to fix test suite failure, which is great, and as such might not strictly require an SRU bug. However, I would create one anyway just to be safe. It would simply need to show the test failure in our CI somewhere, and point to the upstream fix.

The fix for LP: #1901074 is a performance optimization. My experience in trying to SRU optimizations for X11 video drivers was that there is a high reticence for their inclusion versus a regression bug fix. Policies my have changed since then, and it may differ for ruby than X11, but here are some tips of things I've seen raised in the past:

* If possible, tie the performance improvements to use cases actual users would see in practice. Benchmark improvements are interesting but can be viewed as perhaps too theoretical to justify rolling it out to users.

* In particular, how did you learn about the performance issue? If it was raised by a user, or seen on a production system, that would be valuable to mention.

* It looks like some or all of these optimizations are already enabled for other architectures like powerpc64 and amd64. This would be good to mention; in fact it might be better to make the primary justification for this change as "arm64 is missing performance optimizations that other architectures have."

* "Regression Potential" - the SRU team recently renamed this section to clarify what they're looking for is more like "things to watch out for". With performance improvements, particularly in C code, common things to watch for would be segfaults, unexpected behavioral changes, and performance regressions that are seen only on arm64. Mitigating the risk here is that the code paths are already in use for many other architectures.