phobos icon indicating copy to clipboard operation
phobos copied to clipboard

Draft: Reduce template bloat in std.format

Open nordlow opened this issue 2 years ago • 7 comments

This, for instance, reduces memory usage of unittest-checking std.format.write as

dmd -o- -vcolumns -wi -debug -unittest -dip25 -dip1000 -version\=StdUnittest -I/home/per/Work/phobos/ /home/per/Work/phobos/std/format/write.d

from 386 MB to 356 MB.

  1. Motive of added const qualifers is to have, for instance,
format(fmt, int.init)
format(fmt, const(int).init)
format(fmt, immutable(int).init)
...

all result in the same template instance of format.

  1. I've factored out integer printing to make the code not be instantiated for each integral type potentially reducing generated code a lot when formatting and printing integrals. Separate PR at https://github.com/dlang/phobos/pull/8278.

  2. The refactoring in formatValue avoids the need to instantiate the wrapper templates BooleanTypeOf, IntegralTypeOf, etc.

I'm gonna look into other types classes such as floating point types next.

Putting this out into the open to see what CIs feel about this.

Feel free to tell me how you want this split up.

nordlow avatar Sep 19 '21 23:09 nordlow

Thanks for your pull request and interest in making D better, @nordlow! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8247"

dlang-bot avatar Sep 19 '21 23:09 dlang-bot

Irrespective of the memory gains, I think that having the parameter to format being const makes sense on its own as I cannot imagine any scenario where the argument might get modified.

RazvanN7 avatar Sep 21 '21 09:09 RazvanN7

As of 68f0445c2 this PR so far reduces number of instances of formatValue from 10890 to 5459 in make -f posix.mak BUILD=debug unittest. More to come.

Just uncomment line 1233 in std/fomat/write.d and see for yourself.

FYI, @RazvanN7 @andralex @kinke

nordlow avatar Sep 22 '21 16:09 nordlow

@nordlow this is currently failing the testing pipeline.

RazvanN7 avatar Oct 09 '21 09:10 RazvanN7

@nordlow this is currently failing the testing pipeline.

Thanks. I'm gonna branch of minor changes from this. So I'm gonna wait with fixing this.

nordlow avatar Oct 09 '21 09:10 nordlow

This PR currently has two issue that blocks a complete reduction of code bloat in template-variadic formatting and printing functions like std.format.format. These are

auto format(T...)(const(T) args) {}

fails when any of the args is a struct with at least one field being a const class (which is related to the lack of expressing head-mutable class fields in D without resorting to the Rebindable-hack which, btw, contamintes logic in hasAliasing). There is currently no test for this case in Phobos so those need to be added aswell.

auto format(T...)(const T args) {}

fails when any of the args is an InputRange. We might be able to define two overloads of formatValue; one taking a mutable T when isInputRange!(T) and one taking a const T arg when !isInputRange!(T).

These two issues also holds for std.format.text(), write(), writeln(), and trace... functions in std.experimental.logging which are subject to the same parameter qualification as format.

Afaict, both are related to lack of

  • language support for expressing tail-const (head-mutable) pointer and class fields and
  • implicit conversion of a struct S from const to tail-const (head-mutable) when S has any const field being either pointer or class. @andralex has previously eluded to that those rules are subject to relaxation.

I'm personally not sure which alternative is the best long-term.

FYI, @adamdruppe @maxhaton.

nordlow avatar Oct 09 '21 09:10 nordlow

@nordlow I don't know of any plans to introduce tail-constness into the language, so I suggest, in the interest of progress to implement in this PR whatever works and is not dependent on tail-constness. Otherwise, this will be stalled forever.

RazvanN7 avatar Jan 07 '22 12:01 RazvanN7