This is an archive of the discontinued LLVM Phabricator instance.

[libc++][NFC] Refactor the __enable_ifs in <string>
ClosedPublic

Authored by philnik on Feb 22 2023, 9:01 AM.

Details

Summary

This changes all __enable_ifs inside <string> to a common pattern. Specifically, it's always inside the template <> and uses the , int> = 0 style.

Diff Detail

Event Timeline

philnik created this revision.Feb 22 2023, 9:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2023, 9:01 AM
philnik requested review of this revision.Feb 22 2023, 9:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2023, 9:01 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
EricWF requested changes to this revision.Mar 7 2023, 1:54 PM
EricWF added a subscriber: EricWF.

This increases code duplication substantially.

Please say why this refactoring is an improvement.

This revision now requires changes to proceed.Mar 7 2023, 1:54 PM
EricWF added a comment.Mar 7 2023, 1:56 PM

@philnik Every single change needs a description. The description must include the motivation for making the change. What does in improve? What code does it break/fix? etc.

This is important for review, and its even more important down the road when we're trying to figure out why a particular change was made all that time ago.

philnik edited the summary of this revision. (Show Details)Mar 8 2023, 9:23 AM
ldionne accepted this revision.Mar 16 2023, 8:27 AM

I am fine with this since:

  1. It increases consistency with the way we use enable_if elsewhere, and
  2. the duplication is going away once we implement those member functions in the class, which we should do anyway.
libcxx/include/string
2045

There's a bit of duplication here, which is kind of annoying. However, refactoring this short function to be defined in-class will remove that duplication, and also remove some boilerplate which is not really useful.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 19 2023, 2:26 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
alexfh added a subscriber: alexfh.Mar 28 2023, 10:21 AM

This commit increases compilation time by ~15% and increases recursive template instantiation depth (thus, requiring adjusting -ftemplate-depth=2048) in some of our internal test code. Admittedly, it's using templates in a sort of a compiler-load-test-like way, but this change may affect compilation time for real code as well (it's just not likely to hit the default -ftemplate-depth=1024 limit and I don't have a good idea of how to find this code with reasonable effort).

Was this sort of a compiler time regression anticipated with for this change?

This commit increases compilation time by ~15% and increases recursive template instantiation depth (thus, requiring adjusting -ftemplate-depth=2048) in some of our internal test code. Admittedly, it's using templates in a sort of a compiler-load-test-like way, but this change may affect compilation time for real code as well (it's just not likely to hit the default -ftemplate-depth=1024 limit and I don't have a good idea of how to find this code with reasonable effort).

Was this sort of a compiler time regression anticipated with for this change?

I didn't expect really any change w.r.t. compile time. I also don't see how it would affect the amount of recursive template instantiations, since it doesn't add any layers. It just moves the instantiations into the type list. Do you have any specific cases where it changes?

This commit increases compilation time by ~15% and increases recursive template instantiation depth (thus, requiring adjusting -ftemplate-depth=2048) in some of our internal test code. Admittedly, it's using templates in a sort of a compiler-load-test-like way, but this change may affect compilation time for real code as well (it's just not likely to hit the default -ftemplate-depth=1024 limit and I don't have a good idea of how to find this code with reasonable effort).

Was this sort of a compiler time regression anticipated with for this change?

I didn't expect really any change w.r.t. compile time. I also don't see how it would affect the amount of recursive template instantiations, since it doesn't add any layers. It just moves the instantiations into the type list. Do you have any specific cases where it changes?

Yes, I have a specific test, but it will take some time to reduce it and make it shareable. I'm working on this.

So far the only additional piece of diagnostic information I have is a snippet of differences from compilation with -Xclang=-print-stats (- before this patch, + after the patch), which show some differences in the structure of code handled by the compiler:

-    3841 NonTypeTemplateParm decls, 88 each (338008 bytes)
+    4055 NonTypeTemplateParm decls, 88 each (356840 bytes)
...
-    50763 ConstantExpr, 24 each (1218312 bytes)
+    83902 ConstantExpr, 24 each (2013648 bytes)

This commit increases compilation time by ~15% and increases recursive template instantiation depth (thus, requiring adjusting -ftemplate-depth=2048) in some of our internal test code. Admittedly, it's using templates in a sort of a compiler-load-test-like way, but this change may affect compilation time for real code as well (it's just not likely to hit the default -ftemplate-depth=1024 limit and I don't have a good idea of how to find this code with reasonable effort).

I figured out. The test is really a corner case and it's not representative of real code. It artificially creates a chain of 1024 recursive template instantiations and the deepest level started requiring one more template instantiation for std::string after this patch. This is definitely not a problem elsewhere.

As for compilation time regression, it turned out to be much smaller, probably statistically insignificant. The initial difference in compilation time was mostly caused by using different compiler builds. The only consistently reproducible effect on the performance is the aforementioned increase in the number of NonTypeTemplateParm and ConstantExpr and the corresponding increase in memory usage (negligible relative difference, I would say). If you're interested in looking into this, the reduced test code is here: https://gcc.godbolt.org/z/f5xe4nPfK. It demonstrates the extra template instantiation and the increase in the ConstantExpr.