feat: env var to control libxml2 memory management

.github/workflows/ci.yml

@@ -110,9 +110,16 @@ jobs:
       matrix:
         sys: ["enable", "disable"]
         ruby: ["2.7", "3.0", "3.1", "3.2"]
+        mem: ["ruby"]
+        include:
+          - sys: "disable"
+            ruby: "3.2"
+            mem: "default"
     runs-on: ubuntu-latest
     container:
       image: ghcr.io/sparklemotion/nokogiri-test:mri-${{matrix.ruby}}
+    env:
+      NOKOGIRI_LIBXML_MEMORY_MANAGEMENT: ${{matrix.mem}}
     steps:
       - uses: actions/checkout@v3
         with:

CHANGELOG.md

@@ -8,6 +8,22 @@ Nokogiri follows [Semantic Versioning](https://semver.org/), please see the [REA
 
 ### Notes
 
+#### Ability to opt into system `malloc` and `free`
+
+Since 2009, Nokogiri has configured libxml2 to use `ruby_xmalloc` et al for memory management. This has provided benefits for memory management, but comes with a performance penalty.
+
+Users can now opt into using system `malloc` for libxml2 memory management by setting an environment variable:
+
+``` sh
+# "default" here means "libxml2's default" which is system malloc
+NOKOGIRI_LIBXML_MEMORY_MANAGEMENT=default
+```
+
+Benchmarks show that this setting will significantly improve performance, but be aware that the tradeoff may involve poorer memory management including bloated heap sizes and/or OOM conditions.
+
+You can read more about this in the decision record at `adr/2023-04-libxml-memory-management.md`.
+
+
 ### Added
 
 * `Encoding` objects may now be passed to serialization methods like `#to_xml`, `#to_html`, `#serialize`, and `#write_to` to specify the output encoding. Previously only encoding names (strings) were accepted. [[#2774](https://github.com/sparklemotion/nokogiri/issues/2774), [#2798](https://github.com/sparklemotion/nokogiri/issues/2798)] (Thanks, [@ellaklara](https://github.com/ellaklara)!)

adr/2023-04-libxml-memory-management.md

@@ -0,0 +1,109 @@
+
+# 2023-04 Sticking with `ruby_xmalloc` and `ruby_xfree` functions in libxml2
+
+## Status
+
+Affirming the status quo since 2009 -- to use `ruby_xmalloc` et al -- but alternative behavior can be opted into by setting an environment variable:
+
+``` sh
+# "default" here means "libxml2's default" which is system malloc
+NOKOGIRI_LIBXML_MEMORY_MANAGEMENT=default
+```
+
+
+## Context
+
+### Why Nokogiri originally configured libxml2 with `ruby_xmalloc` and `ruby_xfree`
+
+Since 2009, (0dbe1f82), Nokogiri has configured libxml2 to use `ruby_xmalloc` et al for memory operations by making this call in `Init_nokogiri`:
+
+``` c
+  xmlMemSetup(
+    (xmlFreeFunc)ruby_xfree,
+    (xmlMallocFunc)ruby_xmalloc,
+    (xmlReallocFunc)ruby_xrealloc,
+    ruby_strdup);
+```
+
+The reason for doing this is so that Ruby's garbage collection ("GC") subsystem can track the total heap size, including `malloc` calls by C extensions, and is then able to trigger a GC cycle if the total amount of allocated memory exceeds a limit.
+
+@SamSaffron has a great post that explains how this works, and the antipatterns that can emerge if Ruby is not aware of large amount of allocated memory, and I highly recommend that you read it for context:
+
+> [Ruby's external malloc problem - ruby - Sam Saffron's Blog](https://discuss.samsaffron.com/t/rubys-external-malloc-problem/431)
+
+
+## Problems
+
+### Problem: Memory edge cases
+
+We've recently run into a few situations where using `ruby_xmalloc` et al was problematic.
+
+- https://github.com/sparklemotion/nokogiri/issues/2059 and https://github.com/sparklemotion/nokogiri/issues/2241 describe situations where libxml2's `atexit` handler called `ruby_xfree` after ObjectSpace was torn down, causing a segfault
+- https://github.com/sparklemotion/nokogiri/pull/2807 and https://github.com/sparklemotion/nokogiri/issues/2822 describe a situation where Nokogiri's node lifecycle handling causes libxml2 to merge text nodes (calling `ruby_xmalloc` and `ruby_xfree`) while finalizing a Document, preventing the use of `RUBY_TYPED_FREE_IMMEDIATELY` for Documents
+- https://github.com/sparklemotion/nokogiri/issues/2785 describes a situation where libxml2's pthread cleanup code can call `ruby_xfree` after ObjectSpace was torn down, causing a segfault
+
+All the issues have the same root cause: calling `ruby_xfree` in an inappropriate situation, either:
+
+- during GC, or
+- after Ruby's object space has been torn down
+
+These situations would not be inappropriate for using system `malloc` and `free`.
+
+
+### Problem: libxml2 performance
+
+Using `ruby_xmalloc` and `ruby_xfree` has a real performance penalty, as well. Benchmarks at https://github.com/sparklemotion/nokogiri/pull/2843 indicate this penalty can make document parsing up to 34% slower than when the system `malloc` and `free` are used.
+
+
+## Alternatives considered
+
+### System `malloc`
+
+The primary alternative considered is defaulting to using the system `malloc` and `free`.
+
+However, Sam's blog post (as well as other anecdotal data) makes a great case for being extremely careful about the choice of memory management functions.
+
+Without more data, we're declining to change this behavior. But we are introducing the ability to collect some data by providing a runtime option for selecting the memory management suite.
+
+
+### Frankenstein `malloc`
+
+Maybe it's possible to build custom memory management functions that perform better but have some of the benefits of the ruby allocator? This feels well beyond the scope of a C extension.
+
+After an inspection of the ruby memory management functions, it wasn't obvious to the author that there's an obvious performance win by eliminating one or the other of a) conditionally invoking GC if `malloc` fails, or b) tracking the number of bytes allocated using `rb_gc_adjust_memory_usage`.
+
+We would welcome experimental results if other people are motivated to try something like this, though.
+
+
+## Decision
+
+We're sticking with `ruby_xmalloc` et al for now. But we're also introducing an environment variable to allow people to experiment with the system `malloc` if they wish.
+
+
+## Consequences
+
+No changes to the status quo.
+
+
+## References
+
+Memory-related issues:
+
+- https://github.com/sparklemotion/nokogiri/issues/2059 (2020)
+- https://github.com/sparklemotion/nokogiri/issues/2241 (2021)
+- https://github.com/sparklemotion/nokogiri/issues/2785 (2023)
+- https://github.com/sparklemotion/nokogiri/pull/2807 (2023)
+- https://github.com/sparklemotion/nokogiri/issues/2822 (2023)
+
+Upstream libxml2 exit-time issues, commits, and discussion:
+
+- [Fix memory leak when shared libxml.dll is unloaded (!66)](https://gitlab.gnome.org/GNOME/libxml2/-/merge_requests/66)
+- [dlclosing libxml2 with threads crashes (#153)](https://gitlab.gnome.org/GNOME/libxml2/-/issues/153)
+- [Call xmlCleanupParser on ELF destruction (!72)](https://gitlab.gnome.org/GNOME/libxml2/-/merge_requests/72)
+- [Check for custom free function in global destructor (956534e0)](https://gitlab.gnome.org/GNOME/libxml2/-/commit/956534e02ef280795a187c16f6ac04e107f23c5d)
+
+Performance-related discussion:
+
+- https://github.com/sparklemotion/nokogiri/issues/2722
+- https://github.com/sparklemotion/nokogiri/pull/2734
+- https://github.com/sparklemotion/nokogiri/pull/2843

ext/nokogiri/nokogiri.c

@@ -137,6 +137,48 @@ noko_io_close(void *io)
 }
 
 
+#if defined(_WIN32) && !defined(NOKOGIRI_PACKAGED_LIBRARIES)
+#  define NOKOGIRI_WINDOWS_DLLS 1
+#else
+#  define NOKOGIRI_WINDOWS_DLLS 0
+#endif
+
+//
+//   |      dlls || true    | false   |
+//   | nlmm      ||         |         |
+//   |-----------++---------+---------|
+//   | NULL      || default | ruby    |
+//   | "random"  || default | ruby    |
+//   | "ruby"    || ruby    | ruby    |
+//   | "default" || default | default |
+//
+//   We choose *not* to use Ruby's memory management functions with windows DLLs because of this
+//   issue: https://github.com/sparklemotion/nokogiri/issues/2241
+//
+static void
+set_libxml_memory_management(void)
+{
+  const char *nlmm = getenv("NOKOGIRI_LIBXML_MEMORY_MANAGEMENT");
+  if (nlmm) {
+    if (strcmp(nlmm, "default") == 0) {
+      goto libxml_uses_default_memory_management;
+    } else if (strcmp(nlmm, "ruby") == 0) {
+      goto libxml_uses_ruby_memory_management;
+    }
+  }
+  if (NOKOGIRI_WINDOWS_DLLS) {
+libxml_uses_default_memory_management:
+    rb_const_set(mNokogiri, rb_intern("LIBXML_MEMORY_MANAGEMENT"), NOKOGIRI_STR_NEW2("default"));
+    return;
+  } else {
+libxml_uses_ruby_memory_management:
+    rb_const_set(mNokogiri, rb_intern("LIBXML_MEMORY_MANAGEMENT"), NOKOGIRI_STR_NEW2("ruby"));
+    xmlMemSetup((xmlFreeFunc)ruby_xfree, (xmlMallocFunc)ruby_xmalloc, (xmlReallocFunc)ruby_xrealloc, ruby_strdup);
+    return;
+  }
+}
+
+
 void
 Init_nokogiri(void)
 {
@@ -182,26 +224,7 @@ Init_nokogiri(void)
   rb_const_set(mNokogiri, rb_intern("OTHER_LIBRARY_VERSIONS"), NOKOGIRI_STR_NEW2(NOKOGIRI_OTHER_LIBRARY_VERSIONS));
 #endif
 
-#if defined(_WIN32) && !defined(NOKOGIRI_PACKAGED_LIBRARIES)
-  /*
-   *  We choose *not* to do use Ruby's memory management functions with windows DLLs because of this
-   *  issue in libxml 2.9.12:
-   *
-   *    https://github.com/sparklemotion/nokogiri/issues/2241
-   *
-   *  If the atexit() issue gets fixed in a future version of libxml2, then we may be able to skip
-   *  this config only for the specific libxml2 versions 2.9.12.
-   *
-   *  Alternatively, now that Ruby has a generational GC, it might be OK to let libxml2 use its
-   *  default memory management functions (recall that this config was introduced to reduce memory
-   *  bloat and allow Ruby to GC more often); but we should *really* test with production workloads
-   *  before making that kind of a potentially-invasive change.
-   */
-  rb_const_set(mNokogiri, rb_intern("LIBXML_MEMORY_MANAGEMENT"), NOKOGIRI_STR_NEW2("default"));
-#else
-  rb_const_set(mNokogiri, rb_intern("LIBXML_MEMORY_MANAGEMENT"), NOKOGIRI_STR_NEW2("ruby"));
-  xmlMemSetup((xmlFreeFunc)ruby_xfree, (xmlMallocFunc)ruby_xmalloc, (xmlReallocFunc)ruby_xrealloc, ruby_strdup);
-#endif
+  set_libxml_memory_management();
 
   xmlInitParser();
   exsltRegisterAll();