Re: Post release texi2any performance regression

[Gavin Smith]

On Tue, Oct 24, 2023 at 06:15:06PM +0200, Patrice Dumas wrote:
> Maybe it is because of the C code that is run in parallel with perl code?
> That would be my first guess.  Hopefully, the default will be not to run
> C and perl in parallel for the release.  Maybe you could compare with
> a run with 
> TEXINFO_XS=require
> TEXINFO_XS_CONVERT=1
> which would use only C if possible.
> 
> But there could still be places where both are still run, but hopefully
> should be temporary.
> 
> Also, running the C code requires some more perl code, mainly to encode
> strings in UTF-8 before passing to C, this could add some time to run,
> but I doubt that it could be important, as in general this is only done
> once.

There still seems to be lacking any way to turn off the new XS code that
is being run, in order to judge the performance impact.  TEXINFO_XS_PARSER
is now used for a lot of the new XS code, although it is not the parser.

I tried commenting out all the new XS overrides (diff at the end of this
mail) and this seemed to get the execution time back to what it was at
the time of the last release, but I am not convinced it is functioning
properly, as error messages are doubled.

Before (typical time run reported):

$ time ../tp/texi2any.pl ../../emacs-lispref-27.2/elisp.texi 
functions.texi:2390: warning: @inforef is obsolete
errors.texi:226: warning: unexpected argument on @ignore line: The following 
seem to be unused now.

real    0m5.664s
user    0m5.409s
sys     0m0.253s

After (again, I chose typical results out of several):

$ time ../tp/texi2any.pl ../../emacs-lispref-27.2/elisp.texi 
functions.texi:2390: warning: @inforef is obsolete
errors.texi:226: warning: unexpected argument on @ignore line: The following 
seem to be unused now.
functions.texi:2390: warning: @inforef is obsolete
errors.texi:226: warning: unexpected argument on @ignore line: The following 
seem to be unused now.

real    0m5.406s
user    0m5.216s
sys     0m0.188s

These latter timings match more closely the timings of texi2any 7.1.

Even though the error messages are doubled, the Info files appear to
be correctly generated at a first glance.

If I comment out a further block of code, I get rid of the duplicate
errors:

diff --git a/tp/texi2any.pl b/tp/texi2any.pl
index 26cb16e2c0..0eaba5d07b 100755
--- a/tp/texi2any.pl
+++ b/tp/texi2any.pl
@@ -1634,13 +1634,13 @@ while(@input_files) {
 
   $document = Texinfo::Structuring::rebuild_document($document);
 
-  if ($with_XS) {
-    foreach my $error (@{$document->{'errors'}}) {
-      $registrar->add_formatted_message($error);
-    }
-    Texinfo::Structuring::clear_document_errors(
-                                        $document->document_descriptor());
-  }
+#   if ($with_XS) {
+#     foreach my $error (@{$document->{'errors'}}) {
+#       $registrar->add_formatted_message($error);
+#     }
+#     Texinfo::Structuring::clear_document_errors(
+#                                         $document->document_descriptor());
+#   }
 
   $error_count = handle_errors($registrar, $error_count, \@opened_files);
 

(Test suite still doesn't pass with this though - probably a similar
change is needed to tp/t/test_utils.pl but I haven't taken the
time to investigate this.)

(Simply commenting out

$document = Texinfo::Structuring::rebuild_document($document);

improved run times dramatically, but gave incorrect output without
disabling the other XS code.)

Can I please propose that it is made easy to disable all new XS code
in texi2any, as I have done here, so we can avoid losing the performance
of texi2any 7.1.  I don't really care how it's done, as long as TEXINFO_XS
and TEXINFO_XS_PARSER work as before (so TEXINFO_XS_PARSER should be limited
to turning off the XS parser).  It could be configured with a new variable
TEXINFO_XS_MISC or TEXINFO_XS_NEW, or depend on TEXINFO_XS_CONVERT.

It worries me that the new code is entwined with the program and hard to
disable, and could lead to permanent slowdowns if the work doesn't proceed
as hoped.  It would seem much safer for the development of the program
to separate it and be able to disable it cleanly.

I expect that in the future, if XS converters actually do run, then
these changes will be worth it, but they should only be turned on
conditionally.


diff --git a/tp/Texinfo/Common.pm b/tp/Texinfo/Common.pm
index 01c5c118eb..e3f554b83f 100644
--- a/tp/Texinfo/Common.pm
+++ b/tp/Texinfo/Common.pm
@@ -83,32 +83,32 @@ sub import {
   if (!$module_loaded) {
     if (!defined $ENV{TEXINFO_XS_PARSER}
         or $ENV{TEXINFO_XS_PARSER} eq '1') {
-      Texinfo::XSLoader::override(
-        "Texinfo::Common::set_document_options",
-        "Texinfo::StructTransf::set_document_options");
-      Texinfo::XSLoader::override(
-        "Texinfo::Common::copy_tree",
-        "Texinfo::StructTransf::copy_tree");
-      Texinfo::XSLoader::override(
-        "Texinfo::Common::relate_index_entries_to_table_items_in_tree",
-        "Texinfo::StructTransf::relate_index_entries_to_table_items_in_tree"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Common::move_index_entries_after_items_in_tree",
-        "Texinfo::StructTransf::move_index_entries_after_items_in_tree"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Common::protect_colon_in_tree",
-        "Texinfo::StructTransf::protect_colon_in_tree"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Common::protect_comma_in_tree",
-        "Texinfo::StructTransf::protect_comma_in_tree"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Common::protect_node_after_label_in_tree",
-        "Texinfo::StructTransf::protect_node_after_label_in_tree"
-      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Common::set_document_options",
+#        "Texinfo::StructTransf::set_document_options");
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Common::copy_tree",
+#        "Texinfo::StructTransf::copy_tree");
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Common::relate_index_entries_to_table_items_in_tree",
+#        "Texinfo::StructTransf::relate_index_entries_to_table_items_in_tree"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Common::move_index_entries_after_items_in_tree",
+#        "Texinfo::StructTransf::move_index_entries_after_items_in_tree"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Common::protect_colon_in_tree",
+#        "Texinfo::StructTransf::protect_colon_in_tree"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Common::protect_comma_in_tree",
+#        "Texinfo::StructTransf::protect_comma_in_tree"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Common::protect_node_after_label_in_tree",
+#        "Texinfo::StructTransf::protect_node_after_label_in_tree"
+#      );
     }
     $module_loaded = 1;
   }
diff --git a/tp/Texinfo/Convert/HTML.pm b/tp/Texinfo/Convert/HTML.pm
index 1099187716..0832c6c833 100644
--- a/tp/Texinfo/Convert/HTML.pm
+++ b/tp/Texinfo/Convert/HTML.pm
@@ -87,43 +87,43 @@ $VERSION = '7.1';
 our $module_loaded = 0;
 sub import {
   if (!$module_loaded) {
-    Texinfo::XSLoader::override(
-      "Texinfo::Convert::HTML::_default_format_protect_text",
-      "Texinfo::MiscXS::default_format_protect_text");
-    Texinfo::XSLoader::override(
-      "Texinfo::Convert::HTML::_entity_text",
-      "Texinfo::MiscXS::entity_text");
-
-    Texinfo::XSLoader::override(
-      "Texinfo::Convert::HTML::_XS_converter_initialize",
-      "Texinfo::Convert::ConvertXS::html_converter_initialize_sv");
-    Texinfo::XSLoader::override(
-      "Texinfo::Convert::HTML::_XS_initialize_output_state",
-      "Texinfo::Convert::ConvertXS::html_initialize_output_state");
-    Texinfo::XSLoader::override(
-      "Texinfo::Convert::HTML::_XS_sort_sortable_index_entries_by_letter",
-      "Texinfo::Convert::ConvertXS::sort_sortable_index_entries_by_letter");
-    Texinfo::XSLoader::override(
-      "Texinfo::Convert::HTML::_XS_get_index_entries_sorted_by_letter",
-      "Texinfo::Convert::ConvertXS::get_index_entries_sorted_by_letter");
-    Texinfo::XSLoader::override(
-      "Texinfo::Convert::HTML::_XS_prepare_conversion_units",
-      "Texinfo::Convert::ConvertXS::html_prepare_conversion_units");
-    Texinfo::XSLoader::override(
-      "Texinfo::Convert::HTML::_XS_prepare_units_directions_files",
-      "Texinfo::Convert::ConvertXS::html_prepare_units_directions_files");
-    Texinfo::XSLoader::override(
-      "Texinfo::Convert::HTML::_XS_prepare_output_units_global_targets",
-      "Texinfo::Convert::ConvertXS::html_prepare_output_units_global_targets");
-    Texinfo::XSLoader::override(
-      "Texinfo::Convert::HTML::_XS_translate_names",
-      "Texinfo::Convert::ConvertXS::html_translate_names");
-    Texinfo::XSLoader::override(
-      "Texinfo::Convert::HTML::_XS_html_convert_init",
-      "Texinfo::Convert::ConvertXS::html_convert_init");
-    Texinfo::XSLoader::override(
-      "Texinfo::Convert::HTML::_XS_html_convert_convert",
-      "Texinfo::Convert::ConvertXS::html_convert_convert");
+#    Texinfo::XSLoader::override(
+#      "Texinfo::Convert::HTML::_default_format_protect_text",
+#      "Texinfo::MiscXS::default_format_protect_text");
+#    Texinfo::XSLoader::override(
+#      "Texinfo::Convert::HTML::_entity_text",
+#      "Texinfo::MiscXS::entity_text");
+#
+#    Texinfo::XSLoader::override(
+#      "Texinfo::Convert::HTML::_XS_converter_initialize",
+#      "Texinfo::Convert::ConvertXS::html_converter_initialize_sv");
+#    Texinfo::XSLoader::override(
+#      "Texinfo::Convert::HTML::_XS_initialize_output_state",
+#      "Texinfo::Convert::ConvertXS::html_initialize_output_state");
+#    Texinfo::XSLoader::override(
+#      "Texinfo::Convert::HTML::_XS_sort_sortable_index_entries_by_letter",
+#      "Texinfo::Convert::ConvertXS::sort_sortable_index_entries_by_letter");
+#    Texinfo::XSLoader::override(
+#      "Texinfo::Convert::HTML::_XS_get_index_entries_sorted_by_letter",
+#      "Texinfo::Convert::ConvertXS::get_index_entries_sorted_by_letter");
+#    Texinfo::XSLoader::override(
+#      "Texinfo::Convert::HTML::_XS_prepare_conversion_units",
+#      "Texinfo::Convert::ConvertXS::html_prepare_conversion_units");
+#    Texinfo::XSLoader::override(
+#      "Texinfo::Convert::HTML::_XS_prepare_units_directions_files",
+#      "Texinfo::Convert::ConvertXS::html_prepare_units_directions_files");
+#    Texinfo::XSLoader::override(
+#      "Texinfo::Convert::HTML::_XS_prepare_output_units_global_targets",
+#      
"Texinfo::Convert::ConvertXS::html_prepare_output_units_global_targets");
+#    Texinfo::XSLoader::override(
+#      "Texinfo::Convert::HTML::_XS_translate_names",
+#      "Texinfo::Convert::ConvertXS::html_translate_names");
+#    Texinfo::XSLoader::override(
+#      "Texinfo::Convert::HTML::_XS_html_convert_init",
+#      "Texinfo::Convert::ConvertXS::html_convert_init");
+#    Texinfo::XSLoader::override(
+#      "Texinfo::Convert::HTML::_XS_html_convert_convert",
+#      "Texinfo::Convert::ConvertXS::html_convert_convert");
 
     $module_loaded = 1;
   }
diff --git a/tp/Texinfo/Document.pm b/tp/Texinfo/Document.pm
index 8b12698591..037e1b71c4 100644
--- a/tp/Texinfo/Document.pm
+++ b/tp/Texinfo/Document.pm
@@ -33,14 +33,14 @@ use Texinfo::StructTransf;
 our $module_loaded = 0;
 sub import {
   if (!$module_loaded) {
-    if (!defined $ENV{TEXINFO_XS_PARSER}
-        or $ENV{TEXINFO_XS_PARSER} eq '1') {
-      Texinfo::XSLoader::override(
-        "Texinfo::Document::remove_document",
-        # TODO add a Document XS .xs file and move to that file?
-        "Texinfo::StructTransf::remove_document"
-      );
-    }
+    # if (!defined $ENV{TEXINFO_XS_PARSER}
+    #     or $ENV{TEXINFO_XS_PARSER} eq '1') {
+    #   Texinfo::XSLoader::override(
+    #     "Texinfo::Document::remove_document",
+    #     # TODO add a Document XS .xs file and move to that file?
+    #     "Texinfo::StructTransf::remove_document"
+    #   );
+    # }
     $module_loaded = 1;
   }
   # The usual import method
diff --git a/tp/Texinfo/Structuring.pm b/tp/Texinfo/Structuring.pm
index 2b5b4f5a86..6af8e760d2 100644
--- a/tp/Texinfo/Structuring.pm
+++ b/tp/Texinfo/Structuring.pm
@@ -87,66 +87,66 @@ sub import {
   if (!$module_loaded) {
     if (!defined $ENV{TEXINFO_XS_PARSER}
         or $ENV{TEXINFO_XS_PARSER} ne '0') {
-      Texinfo::XSLoader::override(
-        "Texinfo::Structuring::rebuild_document",
-        "Texinfo::StructTransf::rebuild_document");
-      Texinfo::XSLoader::override(
-        "Texinfo::Structuring::rebuild_tree",
-        "Texinfo::StructTransf::rebuild_tree");
-      Texinfo::XSLoader::override(
-        "Texinfo::Structuring::clear_document_errors",
-        "Texinfo::StructTransf::clear_document_errors");
-      Texinfo::XSLoader::override(
-        "Texinfo::Structuring::associate_internal_references",
-        "Texinfo::StructTransf::associate_internal_references"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Structuring::sectioning_structure",
-        "Texinfo::StructTransf::sectioning_structure"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Structuring::warn_non_empty_parts",
-        "Texinfo::StructTransf::warn_non_empty_parts"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Structuring::nodes_tree",
-        "Texinfo::StructTransf::nodes_tree"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Structuring::set_menus_node_directions",
-        "Texinfo::StructTransf::set_menus_node_directions"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Structuring::complete_node_tree_with_menus",
-        "Texinfo::StructTransf::complete_node_tree_with_menus"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Structuring::check_nodes_are_referenced",
-        "Texinfo::StructTransf::check_nodes_are_referenced"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Structuring::number_floats",
-        "Texinfo::StructTransf::number_floats"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Structuring::rebuild_output_units",
-        "Texinfo::StructTransf::rebuild_output_units");
-      Texinfo::XSLoader::override(
-        "Texinfo::Structuring::_XS_unsplit",
-        "Texinfo::StructTransf::unsplit");
-      # Not useful for HTML as functions, as the calling functions are
-      # already overriden
-      # Could be readded when other converters than HTML are done in C
-      #Texinfo::XSLoader::override(
-      #  "Texinfo::Structuring::split_by_node",
-      #  "Texinfo::StructTransf::split_by_node");
-      #Texinfo::XSLoader::override(
-      #  "Texinfo::Structuring::split_by_section",
-      #  "Texinfo::StructTransf::split_by_section");
-      #Texinfo::XSLoader::override(
-      #  "Texinfo::Structuring::split_pages",
-      #  "Texinfo::StructTransf::split_pages"
-      #);
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Structuring::rebuild_document",
+#        "Texinfo::StructTransf::rebuild_document");
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Structuring::rebuild_tree",
+#        "Texinfo::StructTransf::rebuild_tree");
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Structuring::clear_document_errors",
+#        "Texinfo::StructTransf::clear_document_errors");
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Structuring::associate_internal_references",
+#        "Texinfo::StructTransf::associate_internal_references"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Structuring::sectioning_structure",
+#        "Texinfo::StructTransf::sectioning_structure"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Structuring::warn_non_empty_parts",
+#        "Texinfo::StructTransf::warn_non_empty_parts"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Structuring::nodes_tree",
+#        "Texinfo::StructTransf::nodes_tree"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Structuring::set_menus_node_directions",
+#        "Texinfo::StructTransf::set_menus_node_directions"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Structuring::complete_node_tree_with_menus",
+#        "Texinfo::StructTransf::complete_node_tree_with_menus"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Structuring::check_nodes_are_referenced",
+#        "Texinfo::StructTransf::check_nodes_are_referenced"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Structuring::number_floats",
+#        "Texinfo::StructTransf::number_floats"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Structuring::rebuild_output_units",
+#        "Texinfo::StructTransf::rebuild_output_units");
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Structuring::_XS_unsplit",
+#        "Texinfo::StructTransf::unsplit");
+#      # Not useful for HTML as functions, as the calling functions are
+#      # already overriden
+#      # Could be readded when other converters than HTML are done in C
+#      #Texinfo::XSLoader::override(
+#      #  "Texinfo::Structuring::split_by_node",
+#      #  "Texinfo::StructTransf::split_by_node");
+#      #Texinfo::XSLoader::override(
+#      #  "Texinfo::Structuring::split_by_section",
+#      #  "Texinfo::StructTransf::split_by_section");
+#      #Texinfo::XSLoader::override(
+#      #  "Texinfo::Structuring::split_pages",
+#      #  "Texinfo::StructTransf::split_pages"
+#      #);
     }
     $module_loaded = 1;
   }
diff --git a/tp/Texinfo/Transformations.pm b/tp/Texinfo/Transformations.pm
index 836e1b4aae..0a98e2016b 100644
--- a/tp/Texinfo/Transformations.pm
+++ b/tp/Texinfo/Transformations.pm
@@ -54,41 +54,41 @@ $VERSION = '7.1';
 our $module_loaded = 0;
 sub import {
   if (!$module_loaded) {
-    if (!defined $ENV{TEXINFO_XS_PARSER}
-        or $ENV{TEXINFO_XS_PARSER} eq '1') {
-      Texinfo::XSLoader::override(
-        "Texinfo::Transformations::fill_gaps_in_sectioning",
-        "Texinfo::StructTransf::fill_gaps_in_sectioning"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Transformations::reference_to_arg_in_tree",
-        "Texinfo::StructTransf::reference_to_arg_in_tree"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Transformations::complete_tree_nodes_menus",
-        "Texinfo::StructTransf::complete_tree_nodes_menus"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Transformations::complete_tree_nodes_missing_menu",
-        "Texinfo::StructTransf::complete_tree_nodes_missing_menu"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Transformations::regenerate_master_menu",
-        "Texinfo::StructTransf::regenerate_master_menu"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Transformations::insert_nodes_for_sectioning_commands",
-        "Texinfo::StructTransf::insert_nodes_for_sectioning_commands"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Transformations::protect_hashchar_at_line_beginning",
-        "Texinfo::StructTransf::protect_hashchar_at_line_beginning"
-      );
-      Texinfo::XSLoader::override(
-        "Texinfo::Transformations::protect_first_parenthesis_in_targets",
-        "Texinfo::StructTransf::protect_first_parenthesis_in_targets"
-      );
-    }
+#    if (!defined $ENV{TEXINFO_XS_PARSER}
+#        or $ENV{TEXINFO_XS_PARSER} eq '1') {
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Transformations::fill_gaps_in_sectioning",
+#        "Texinfo::StructTransf::fill_gaps_in_sectioning"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Transformations::reference_to_arg_in_tree",
+#        "Texinfo::StructTransf::reference_to_arg_in_tree"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Transformations::complete_tree_nodes_menus",
+#        "Texinfo::StructTransf::complete_tree_nodes_menus"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Transformations::complete_tree_nodes_missing_menu",
+#        "Texinfo::StructTransf::complete_tree_nodes_missing_menu"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Transformations::regenerate_master_menu",
+#        "Texinfo::StructTransf::regenerate_master_menu"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Transformations::insert_nodes_for_sectioning_commands",
+#        "Texinfo::StructTransf::insert_nodes_for_sectioning_commands"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Transformations::protect_hashchar_at_line_beginning",
+#        "Texinfo::StructTransf::protect_hashchar_at_line_beginning"
+#      );
+#      Texinfo::XSLoader::override(
+#        "Texinfo::Transformations::protect_first_parenthesis_in_targets",
+#        "Texinfo::StructTransf::protect_first_parenthesis_in_targets"
+#      );
+#    }
     $module_loaded = 1;
   }
   # The usual import method
diff --git a/tp/Texinfo/TranslationsNonXS.pm b/tp/Texinfo/TranslationsNonXS.pm
index 5e9af44afe..283bfb8d59 100644
--- a/tp/Texinfo/TranslationsNonXS.pm
+++ b/tp/Texinfo/TranslationsNonXS.pm
@@ -47,12 +47,12 @@ Locale::Messages->select_package ('gettext_pp');
 our $module_loaded = 0;
 sub import {
   if (!$module_loaded) {
-    # the first one is defined in this here perl file.
-    # the second one is defined in the Translations.xs file.
-    Texinfo::XSLoader::override(
-      "Texinfo::Translations::_configure_XS",
-      "Texinfo::Translations::configure_XS"
-    );
+#    # the first one is defined in this here perl file.
+#    # the second one is defined in the Translations.xs file.
+#    Texinfo::XSLoader::override(
+#      "Texinfo::Translations::_configure_XS",
+#      "Texinfo::Translations::configure_XS"
+#    );
     $module_loaded = 1;
   }
   # The usual import method