Separate semantic routines from expression.d

[RazvanN7]

This PR continues the work started in [1] and continued in [2] by separating the semantic routines from the ast nodes for expression nodes.

[1] #5730
[2] #7007

[dlang-bot]

Thanks for your pull request, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

• smaller, focused PRs are easier to review than big ones

• try not to mix up refactoring or style changes with bug fixes or feature enhancements

• provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

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.

[RazvanN7]

I was a bit worried about the compilation overhead due to the extra function call, so I made some measurements: I timed the standard make -f posix.mak for druntime and phobos (10 times each, for each version) and the results show that the overhead is completely neglectable.

Compiling druntime:

master:
-> minimum compilation time : 7.168 s
-> maximum compilation time : 8.342 s
-> mean compilation time : 8.267 s

PR:
-> minimum compilation time : 8.371 s
-> maximum compilation time : 8.470 s
-> mean compilation time : 8.405 s

Compiling phobos:

master:
-> minimum compilation time : 17.785 s
-> maximum compilation time : 17.879 s
-> mean compilation time : 17.832 s

PR:
-> minimum compilation time : 17.891 s
-> maximum compilation time : 17.942 s
-> mean compilation time : 17.907s

[andralex]

Nice! I'm a bit worried about the minima for druntime, but those for phobos look good. @WalterBright are these the droids you're looking for?

[WalterBright]

Nice work, but the 4% slowdown is a bit disheartening. Andrei, is there another way to do the visitor pattern?

[ibuclaw]

The entrypoint semantic function should be exposed to c++ perhaps as an interim, otherwise how am I going to be able to call it?

[ibuclaw]

Or maybe there should be a global function table that exposes these entrypoint routines to C++.

[RazvanN7]

@andralex @WalterBright the minima for druntime on master (7.168 s) was obtained only once and the rest 9 measurements were 8,3.. . It is very possible that the difference is due to operating system specifics and not compilation overhead.

Anyway, using the visitor pattern implies two function calls (double dispatch), so the only workaround for this would be to just get rid of the visitor and implement the semantic methods as an overload set, but that might be complicated given the inheritance relationship between classes.

[RazvanN7]

@ibuclaw If this PR is merged, I was thinking of creating a file named semantic.d which imports all the *sem.d modules and creates an overload set with the semantic routines from all those modules. After this is done, I will create the semantic.h file which has the headers. How's that?

[andralex]

Nice work, but the 4% slowdown is a bit disheartening. Andrei, is there another way to do the visitor pattern?

The obvious way to do it is to (a) leave all semantic-related member variables in the respective classes, and (b) collect all semantic function implementations in a module. The latter is possible in C++ but not in D (you can't define a method out-of-class). There are of course obvious disadvantages, but that would only cost one virtual call.

But let's take a second look at the measurements as it's very important how they're conducted.

@andralex @WalterBright the minima for druntime on master (7.168 s) was obtained only once and the rest 9 measurements were 8,3.. . It is very possible that the difference is due to operating system specifics and not compilation overhead.

(I assume measurements were conducted on the same OS so not sure I get the last sentence.) In the presence of noise, the best way to measure differences in timings is to run both experiments very many times and then compare the modes (mode = the point where the density of samples is the highest). But getting a reliable mode would necessitate a bunch of experiments i.e. a long time.

A possibility is to look at the same order statistic (e.g. median, 90 percentile etc). Again that takes a fair number of measurements to get.

So then we go with the notion that the noise is fluctuating, occasional, and always additive (no noise reduces computation time). By that hypothesis we get to the conclusion that mode is close to the shortest time, because that is the one least affected by noise. Even if noise is not occasional, the minimum is most indicative of the computation itself. That's why comparing minima is better than the average (VERY noisy).

I ssh'd into a quiet machine otherwise unused and ran the following experiments:

cd /path/to/druntime
repeat 50 { make -j -f posix.mak clean >/dev/null && time make -j1 -f posix.mak >/dev/null }

For master I got:

make -j1 -f posix.mak > /dev/null  4.95s user 0.16s system 93% cpu 5.479 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.15s system 93% cpu 5.538 total
make -j1 -f posix.mak > /dev/null  5.01s user 0.17s system 93% cpu 5.551 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.14s system 93% cpu 5.518 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.12s system 93% cpu 5.494 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.13s system 93% cpu 5.509 total
make -j1 -f posix.mak > /dev/null  5.10s user 0.11s system 93% cpu 5.578 total
make -j1 -f posix.mak > /dev/null  5.01s user 0.19s system 93% cpu 5.572 total
make -j1 -f posix.mak > /dev/null  4.97s user 0.16s system 93% cpu 5.490 total
make -j1 -f posix.mak > /dev/null  4.96s user 0.15s system 93% cpu 5.481 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.16s system 93% cpu 5.545 total
make -j1 -f posix.mak > /dev/null  4.96s user 0.14s system 93% cpu 5.477 total
make -j1 -f posix.mak > /dev/null  5.06s user 0.14s system 93% cpu 5.560 total
make -j1 -f posix.mak > /dev/null  4.95s user 0.16s system 93% cpu 5.482 total
make -j1 -f posix.mak > /dev/null  5.03s user 0.11s system 93% cpu 5.513 total
make -j1 -f posix.mak > /dev/null  5.05s user 0.14s system 93% cpu 5.557 total
make -j1 -f posix.mak > /dev/null  4.96s user 0.16s system 93% cpu 5.494 total
make -j1 -f posix.mak > /dev/null  4.99s user 0.13s system 93% cpu 5.494 total
make -j1 -f posix.mak > /dev/null  5.05s user 0.12s system 93% cpu 5.538 total
make -j1 -f posix.mak > /dev/null  5.04s user 0.12s system 93% cpu 5.542 total
make -j1 -f posix.mak > /dev/null  4.97s user 0.14s system 93% cpu 5.479 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.15s system 93% cpu 5.523 total
make -j1 -f posix.mak > /dev/null  4.99s user 0.13s system 93% cpu 5.489 total
make -j1 -f posix.mak > /dev/null  4.99s user 0.14s system 93% cpu 5.512 total
make -j1 -f posix.mak > /dev/null  4.96s user 0.17s system 93% cpu 5.505 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.12s system 93% cpu 5.517 total
make -j1 -f posix.mak > /dev/null  4.95s user 0.18s system 93% cpu 5.506 total
make -j1 -f posix.mak > /dev/null  5.01s user 0.13s system 93% cpu 5.508 total
make -j1 -f posix.mak > /dev/null  4.95s user 0.15s system 93% cpu 5.477 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.14s system 93% cpu 5.510 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.13s system 93% cpu 5.504 total
make -j1 -f posix.mak > /dev/null  5.01s user 0.11s system 93% cpu 5.487 total
make -j1 -f posix.mak > /dev/null  5.04s user 0.12s system 93% cpu 5.536 total
make -j1 -f posix.mak > /dev/null  4.97s user 0.16s system 93% cpu 5.514 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.13s system 93% cpu 5.513 total
make -j1 -f posix.mak > /dev/null  5.01s user 0.11s system 93% cpu 5.496 total
make -j1 -f posix.mak > /dev/null  4.99s user 0.12s system 93% cpu 5.491 total
make -j1 -f posix.mak > /dev/null  5.05s user 0.11s system 93% cpu 5.539 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.18s system 93% cpu 5.555 total
make -j1 -f posix.mak > /dev/null  5.04s user 0.14s system 93% cpu 5.549 total
make -j1 -f posix.mak > /dev/null  4.99s user 0.16s system 93% cpu 5.519 total
make -j1 -f posix.mak > /dev/null  5.03s user 0.13s system 93% cpu 5.541 total
make -j1 -f posix.mak > /dev/null  5.08s user 0.09s system 93% cpu 5.549 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.12s system 93% cpu 5.521 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.12s system 93% cpu 5.501 total
make -j1 -f posix.mak > /dev/null  5.01s user 0.16s system 93% cpu 5.532 total
make -j1 -f posix.mak > /dev/null  5.05s user 0.15s system 93% cpu 5.573 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.14s system 93% cpu 5.519 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.14s system 93% cpu 5.531 total
make -j1 -f posix.mak > /dev/null  4.97s user 0.13s system 93% cpu 5.475 total

For this patch I got:

make -j1 -f posix.mak > /dev/null  5.06s user 0.14s system 93% cpu 5.580 total
make -j1 -f posix.mak > /dev/null  5.04s user 0.09s system 93% cpu 5.515 total
make -j1 -f posix.mak > /dev/null  5.05s user 0.11s system 93% cpu 5.542 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.14s system 93% cpu 5.524 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.15s system 93% cpu 5.547 total
make -j1 -f posix.mak > /dev/null  5.06s user 0.10s system 93% cpu 5.545 total
make -j1 -f posix.mak > /dev/null  5.05s user 0.10s system 93% cpu 5.517 total
make -j1 -f posix.mak > /dev/null  4.98s user 0.17s system 93% cpu 5.521 total
make -j1 -f posix.mak > /dev/null  4.94s user 0.17s system 93% cpu 5.488 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.16s system 93% cpu 5.539 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.12s system 93% cpu 5.491 total
make -j1 -f posix.mak > /dev/null  4.99s user 0.16s system 93% cpu 5.517 total
make -j1 -f posix.mak > /dev/null  5.06s user 0.11s system 93% cpu 5.551 total
make -j1 -f posix.mak > /dev/null  4.98s user 0.15s system 93% cpu 5.502 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.12s system 93% cpu 5.512 total
make -j1 -f posix.mak > /dev/null  5.07s user 0.12s system 93% cpu 5.567 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.16s system 93% cpu 5.548 total
make -j1 -f posix.mak > /dev/null  5.04s user 0.10s system 93% cpu 5.513 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.14s system 93% cpu 5.546 total
make -j1 -f posix.mak > /dev/null  4.96s user 0.18s system 93% cpu 5.515 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.15s system 93% cpu 5.523 total
make -j1 -f posix.mak > /dev/null  4.99s user 0.15s system 93% cpu 5.509 total
make -j1 -f posix.mak > /dev/null  5.09s user 0.10s system 93% cpu 5.565 total
make -j1 -f posix.mak > /dev/null  5.01s user 0.14s system 93% cpu 5.525 total
make -j1 -f posix.mak > /dev/null  5.06s user 0.13s system 93% cpu 5.561 total
make -j1 -f posix.mak > /dev/null  5.04s user 0.14s system 93% cpu 5.565 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.14s system 93% cpu 5.510 total
make -j1 -f posix.mak > /dev/null  4.99s user 0.16s system 93% cpu 5.530 total
make -j1 -f posix.mak > /dev/null  5.04s user 0.14s system 93% cpu 5.552 total
make -j1 -f posix.mak > /dev/null  5.03s user 0.15s system 93% cpu 5.555 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.10s system 93% cpu 5.501 total
make -j1 -f posix.mak > /dev/null  4.98s user 0.19s system 93% cpu 5.550 total
make -j1 -f posix.mak > /dev/null  5.09s user 0.13s system 93% cpu 5.591 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.15s system 93% cpu 5.530 total
make -j1 -f posix.mak > /dev/null  5.04s user 0.12s system 93% cpu 5.535 total
make -j1 -f posix.mak > /dev/null  4.96s user 0.14s system 93% cpu 5.472 total
make -j1 -f posix.mak > /dev/null  5.05s user 0.13s system 93% cpu 5.537 total
make -j1 -f posix.mak > /dev/null  5.01s user 0.16s system 93% cpu 5.536 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.15s system 93% cpu 5.540 total
make -j1 -f posix.mak > /dev/null  5.10s user 0.12s system 93% cpu 5.589 total
make -j1 -f posix.mak > /dev/null  4.99s user 0.18s system 93% cpu 5.546 total
make -j1 -f posix.mak > /dev/null  5.06s user 0.12s system 93% cpu 5.541 total
make -j1 -f posix.mak > /dev/null  5.03s user 0.16s system 93% cpu 5.565 total
make -j1 -f posix.mak > /dev/null  5.08s user 0.12s system 93% cpu 5.572 total
make -j1 -f posix.mak > /dev/null  4.98s user 0.16s system 93% cpu 5.513 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.14s system 93% cpu 5.521 total
make -j1 -f posix.mak > /dev/null  5.06s user 0.10s system 93% cpu 5.533 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.14s system 93% cpu 5.547 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.13s system 93% cpu 5.521 total
make -j1 -f posix.mak > /dev/null  5.01s user 0.17s system 93% cpu 5.564 total

For master I got a minimum of 5.477 seconds per build. For this patch I got a minimum of 5.472 which is essentially identical.

My conclusion is there's no need to worry about a performance loss with this PR.

[RazvanN7]

@andralex

(I assume measurements were conducted on the same OS so not sure I get the last sentence.)

The measurements were made on the same OS but it is possible that due to some OS favorable situation (for example: fewer context switches, smaller I/O waiting times etc) the minimum was obtained in exceptional circumstances.

[andralex]

The rule of thumb wrt additive noise is there are exceptional maxima but not exceptional minima. Low minima should be reproducible. Nicely enough they are!

[ibuclaw]

Useless comment.

[ibuclaw]

@ibuclaw If this PR is merged, I was thinking of creating a file named semantic.d which imports all the *sem.d modules and creates an overload set with the semantic routines from all those modules. After this is done, I will create the semantic.h file which has the headers. How's that?

It doesn't help that semantic itself is not extern(C++).

[andralex]

@ibuclaw I assume this is ok to merge now and have @RazvanN7 follow up with the C++ interface? LMK, thanks!

[ibuclaw]

Just the function semantic itself needs to be marked as extern(C++). Variants can remain hidden.

[andralex]

@RazvanN7 lmk when you mind @ibuclaw's need and let's pull this - thx!

[RazvanN7]

@andralex I marked the semantic routine as extern (C++). Let's merge this and move on.

[andralex]

Please rebase

[ibuclaw]

+// entrypoint for semantic ExpressionSemanticVisitor
+extern (C++) Expression semantic(Expression e, Scope* sc)
+{
+    scope v = new ExpressionSemanticVisitor(sc);
+    e.accept(v);
+    return v.result;
+}

Looks ok to me.

[UplinkCoder]

@WalterBright actually we can be the vistors faster by using the op-field or perferbly by storing a type-enum in RootObject.

However the alternative which is keeping the virtual functions for theese things which users are unlilkely to ever hook into is much prefered!

[UplinkCoder]

secondly WHY WAS THIS MERGED ????

[andralex]

@UplinkCoder we are converting virtuals to visitors for compilation steps in order to improve modularity and coupling. I understand this breaks a bunch of ongoing PRs, but it's a cost @WalterBright and I decided is worth taking. Apologies for the extra churn.

[ibuclaw]

@UplinkCoder we are converting virtuals to visitors for compilation steps in order to improve modularity and coupling.

We should probably think about packaging these modules perhaps (ddmd.semantic.expression and others).

I understand this breaks a bunch of ongoing PRs, but it's a cost @WalterBright and I decided is worth taking. Apologies for the extra churn.

We should also dedicate a week to going through these as we do with bugzilla reports. Take ownership of and rebase, or close all PRs below 4000, for instance.