This restores the previous behavior (from before a couple commits ago), but
adds a warning.
We have a GitLab pipeline internally where we break apart certain
directories into separate jobs to parallelize them. There is a bug relating
to build and retaining artifacts for rate tables that only manifests itself
for Summary Pages, and that needs to be resolved before this is removed.
DEV-15153
This accomplishes a couple of major things:
1. Removes the need for package-specific symtable maps, since we can just
use the global linker one; and
2. Stops typesetting things that are not linked.
I didn't realize how much of the latter was happening. On one of our large
programs, the filesize dropped from 64MiB to 51MiB. Consider also that,
because the menu generation _does_ only take into account the linked symbol
list, _these items were never even accessible_, at least through normal
means, so the user couldn't even see them.
Compilation time for that program dropped from ~2m20s to ~1m.
DEV-15153
This begins to chip away at the terrible performance of this system by using
maps where linear scans of the symbol table were otherwise taking place.
This does not yet touch `preproc:sym-deps`, which has the same problem.
Further, we may be able to get rid of symbol table maps per package by
utilizing `l:dep`; more to come.
This trims ~40s off of one of our larger programs (3m -> 2m20s), which is a
start, but there's still a ton of room for improvement. I don't expect it
to be blazingly fast after all the optimization attempts---this has the same
problem as the XSLT-based linker did, which ended up being reimplemented in
Rust as TAMER---but it'll help to improve times in the meantime.
DEV-15153
This was optimized previously, having introduced
`@need-resolve-dtype`. But, while that solved the problem for some
packages, it was not enough for a package with 40,000 symbols.
This introduces a map for typedefs' symbols and eliminates ~15s for that
package.
DEV-15114
And here's the optimization that I had wanted to perform all along, but it
took some time to confidently get to this point.
`preceding-sibling` was used since the XSLT1 days for deduplication, before
`for-each-group` existed. It works just fine for small inputs, but the
problem is we're doing this many thousands of times in larger packages, and
that really adds up. (In this case, shaving ~8s off of one of our large
packages with ~20k symbols in play.)
DEV-15114
See the `preproc:tex-gen` comment for more information. This retains the
existing behavior, cleaning it up quite a bit.
This has no impact on performance; it's just continued refactoring to
prepare for optimization.
DEV-15114
This just continues to refactor to try to make sense of this code, which has
evolved into quite the mess over the years. The two primary goals are
to (a) find possible optimizations and (b) make sure how this functions is
clear for when it's reimplemented in TAMER.
I'm doing this in small commits so that the steps are more obvious.
The specific list of attributes is what I found to be output in practice in
the `xmlo` files.
DEV-15114
I had to dig through the old repository (prior to extracting into this one)
to see why this was introduced. It seems that it was for the linker, and
TAMER's linker has no concept of lax symbols, so this is not used.
To make matters worse, the code I modified here could not have worked (in
depgen) because `$syms/@name` _will always have a value_.
Anyway, removing this has no effect on the compiled packages.
DEV-15114
This does not impact performance, but it makes it less confusing. What a
mess this whole thing is. I'll have to incrementally refactor it until it
makes sense enough to optimize.
For this who don't know, from XSLT 1.0 days: "rtf" means "result tree
fragment", before sequences were a thing, and you had to treat generated
trees specially. Yeah, old code.
DEV-15114
This is an interesting one.
For some context: TAME uses `csvm` files to provide syntactic sugar for
large tables of values ("rate tables", as they're often called, since they
contain insurance rates and other data). This gets desugared into a `csv`
which in turn is compiled via `csv2xml` into a package. That package uses
the `_table-*_` templates to define a table, which is represented as a
matrix using `const/@values`.
Here's an example of a generated table in a package:
```
<t:create-table name="foo">
<t:table-rows data="
1,2,3;
4,5,6;" />
</t:create-table>
```
Some of the tables are quite large, generating tens of MiB of data in
`@data`. This in itself isn't a problem. But when Saxon parses the `@data`
attribute, it normalizes the whitespace, as mandated by the XML spec, and
removes the newlines. Therefore, when the template is expanded and the
`xmlo` file is produced, the template produced a `const/@values` with a huge
amount of data on one line.
Then, when another package imports that `xmlo` file via `<import
package="..." />`, which is done via `document()` in XSLT, Saxon takes a
long time to parse it. 60s on my machine for a ~20MiB line.
This problem does not exist for JS fragments; Saxon doesn't mind large text
nodes. So that is the approach that is taken here.
The template system doesn't have a way to output text yet, so this takes an
approach that minimizes changes as much as possible:
- `param-copy` will expand `with-param/@value` as a text node.
- `const/@values="-"` will cause TAME to use the child text node as the
value of `@values`.
- `_table-rows_` is modified to use the above two features.
The reason for using `@values="-"` is so that other parts of the compiler do
not have to be modified to recognize the new text convention, which is
otherwise awkward because newlines are text nodes. The `-` convention comes
from command line programs, which means "read from stdin", generally; this
is okay since `-` is never a valid matrix specification.
This must have been a problem for a very long time, but wasn't all that
noticeable until recent performance optimizations, since so many other things
around it were also slow.
DEV-15131
It's embarrassing how much time this saved on builds.
This apparently has always been doing a linear scan on the entire symbol
table for _every single param in the symbol table_, including those that
were imported. This is not only unnecessary, but has no effect on the end
result of the system.
This cut build times almost in half, due to the number of symbols in some of
our packages. All for unnecessary work.
Like most things that have quadratic (or polynomial) time complexity, they
don't show up during initial development, and are hard to even profile for,
because their effects are so small. Now that our system has grown
substantially, it had a massive effect.
DEV-15114
This should have been part of a previous commit. So few parts of the system
utilize tests that I didn't notice that this was failing until after the
commit, since I'm not used to running them...even though I wrote them.
This system is being replaced with TAMER, so these changes largely just get
the tests working properly again.
DEV-15095
This is more of the same, utilizing the map I created previously. The
results are pretty significant; this commit and the previous cuts ~3.5m of
build time (if done serially) off of our largest system.
My goal is to get non-parallelizable portions of our build down to the point
where they are no longer the bottleneck. This just about does that.
DEV-15095
I wanted to get this committed before I continue because it required changes
to the `expand-sequence` system---tunneling params cannot pass through
functions, so this accepts a context to pass back to the calling system via
the `eseq:expand-node` override.
Otherwise, the key change here is the elimination of a preproc:symtable
XPath within a `template/@match`, which was a huge performance problem with
the preceding commits. This improves build times modestly, but there are
more changes that this sets up for, so I'll keep going.
DEV-15095
This uses the already-available symtable-map to avoid expensive XPaths
resulting in (what I assume to be) linear scans of the symbol table.
This effectively makes the fragment compilation time vanish.
This had the effect of shaving ~4.5m total off of our largest system (if I
were to do `-j1`), and a couple minutes when run in parallel.
DEV-15095
This takes a bunch of individual templates and combines them into one, while
also utilizing the already-available symbol table map in place of using an
XPath on `preproc:symtable`.
The results are much more drastic than I was expecting.
I was exploring this because one of our largest packages was spending most
of its time (~5m) in fragment compilation, which was a surprise to
me. Prior work I did for runtime optimizations led to optimizations in its
parent `js.xsl`, but not in `js-calc.xsl`, which has largely been untouched
since it was originally written for XSLT 1 over 10 years ago.
Because it was originally written for XLST 1, it does not take advantage of
maps, tunneling variables, or various other options. Further, it was
written in a naive way that was convenient (and clear) at the time, and
wholly acceptable for smaller inputs. But, as is the case with
quadratic-time systems, there are severe growing pains.
This change reduced the package compilation time from 5-6m down to 1m15s,
and this was just the first attempt at optimizing it. I should have taken a
look at this long ago, but my efforts were focused on TAMER, and I didn't
want to divert that focus. That was a mistake. Symptoms of this problem
were already prevalent ~10 months ago, when the package was taking 3 minutes
to compile (so the time has since doubled).
This also eliminates `@magic`, which has not been used for a long time (it
used to be used for a "constant" that held the current date/time; such a
value is now passed into the system like any other input).
After making this change, the resulting packages are byte-for-byte
identical.
I also noticed, though I haven't tried to measure it, that there seem to be
fewer multi-core spikes; this is possibly related to Saxon not trying to
evaluate expensive `template/match` expressions concurrently anymore. If
true, this will also help with resource contention for parallel builds.
DEV-15095
This is a rather small change for quite a bit of effort in researching what
was going wrong. It's at last seven rabbit holes deep, or maybe several
herd of yaks, depending on your choice of measure and the current conversion
rate.
The problem can be summarized fair succinctly:
`expand-sequence/expand-group` exists to prevent an expansion repass for
every single child element of the `expand-sequence`, which would be
quadratic. Basically, it restores the usual template expansion process for
that set of children.
But apparently `expand-group` was stripped on the first pass, which expanded
its children inline, which then meant that each of the children were subject
to their own individual passes, defeating the purpose of the optimization.
As is the nature of quadratic-time processes, that was not noticed until
inputs became especially large, and not only that, but were combined with
nested `expand-sequence`s.
I would say that this never worked the way that I intended it to, but I'm
not certain. I was working quite a bit with TeX at the time, so it's
possible that I modeled it after `\expandafter`. But that's not an
appropriate model for TAME.
TAMER will be removing expand-sequence entirely, since it will have enough
of an understanding of the source system to determine what requires
expansion and what requires ordering (e.g. for symbol table
iteration). I'll also be making changes to simplify the process by further
restricting what type of iteration can take place. But for the time being,
the change was necessary.
In our largest systems, this change cut off ~15m total of build time if run
serially (at `-j1`). After sorting two runtabs for comparison (e.g. `sort
-k4`), you can get the total like so:
$ paste <( sort -k4 runtab-a ) <( sort -k4 runtab-b )
| grep xmlo\$ \
| cut -f2,5,6 \
| awk '{ total += ($1 - $2) } END { print total / 1000 }'
Similarly, this Awk expression will give the time differences:
$ awk '{ print ($1 - $2)/1000, $5 }'
Further, the previous commit also introduced a `xmle-sym-cmp` tool to check
for differences between xmle symbol tables in an automated way, irrespective
of ordering (since there are many valid topological sorts). This revealed
that the change fixed a bug (likely because of the forced repass after
`expand-group` hoisting) that was causing symbol table introspection to fail
to discover symbols in certain cases, which in our case, was resulting in
the failure to generate a small number of aggregate classifications
correctly. The whole repass system is a concerning mess, but it's not worth
the effort to try to redo all of that when that work can be done in TAMER.
DEV-15069
See the description for more information. This takes place of manual
diffing that I've been doing over the years, which is error-prone and
time-consuming.
Note that a successful diff doesn't mean "we're good!"; other things could
have failed. This specifically checks xmle symbol sets, nothing more.
DEV-15069
When set, this will cause runner ACK failures to occur approximately every
1/N requests. This allows manually testing conditions that would normally
only occur under severe resource contention.
DEV-10806
When the client requests a reload (e.g. on ACK wait failure), we should not
terminate, since we are expecting to attempt the request again.
This was broken by the previous commit.
DEV-10806
It's a tad embarrassing that this has been eluding me for quite some
time. I happened to run into it while testing the previous commit, which in
turn only existed because I was trying to optimize runner performance.
We'd have situations where, following a runner reload (exit code 129 =
SIGHUP), the build would simply hang indefinitely. Apparently, `tame`, in
`command-runner`, blocks on a `read` without a timeout, expecting that the
FIFO attached to stdin will close if the runner crashes. Maybe that used to
be the case, but that is no longer true today.
Because of that, the FIFO stays open, and read continues to block, waiting
for `DONE`.
Now, `tamed`, when seeing that a runner has crashed (which could have been
due to a reload), will check to see if that runner is marked busy. If so,
that means that the client `tame` did not see `DONE`, because it did not
clear the flag via `command-runner`'s `mark-available.` To notify the
client that someone went wrong, `tamed` will inject a `DONE` into the output
FIFO, which will allow the client to fail properly.
`dslc` catches exceptions and should output `DONE` under normal operating
conditions. However, since some of our systems require so much memory to
build, we may encounter the OOM killer. In that case, the process has no
time to recover (it is killed with SIGKILL), and therefore cannot output
`DONE`. I suspect this is what has been happening to cause occasional build
hangs.
One final thing to clean this up: since we're properly handling reloads now,
based on this commit and the immediately preceding one, we can suppress the
warning when the code is 129 (see comments).
DEV-10806
The `tame` client has the ability to request a runner reload by issuing
SIGHUP to the runner PID. This will cause `tamed` to kill the runner and
respawn it.
There were situations where this process hung or did not operate as
expected; the process was not reliable. This does a number of things to
make sure the process is in a good state before proceeding:
1. The HUP trap is set a single time, rather than being reset each time
the signal is received for the runner.
2. A `reloading` flag is set by the `tame` client when reloading is
requested, and the flag is cleared by `tamed` when spawning a new
runner. This allows the client to wait until the reload is complete,
and bail out otherwise. Without this, there is a race, where the
client may proceed to issue additional requests before the original
runner terminates.
3. Kill the runner with SIGKILL (signal 9). This gives no opportunity for
the runner to ignore the signal, and gives us confidence that the
runner is actually going to be killed.
This may have caused errors that look like this (where 129 is the HUP
reload, which is expected):
warning: failed runner 0 ack; requesting reload
warning: runner 0 exited with code 129; restarting
error: runner 0 still unresponsive; giving up
The last line may also be omitted, with instead an empty `xmlo` being
generated.
DEV-10806
This uses `exec` so that the process is replaced, simplifying process
management (we don't have to worry that the script will die but leave the
child hanging). I'm not sure why I didn't do this originally.
DEV-10806
I have long forgotten about this system. It converts typedefs into a more
generic domain, but the way in which it does so causes duplicate domains,
for two reasons:
- Both `preproc:mkdomain` and the caller (`preproc:expand`) recurse into
unions and generate domains; and
- Each `preproc:expand` pass generates domains.
So, for example, if there are two `preproc:expand` passes on a union, then
the outer typedef (union) will have domains generated twice (once for each
pass), and the inner typedefs will have domains generated four times (for
each expansion pass, and twice for each pass).
This resolves the issue before the next commit makes further changes to move
this into a generated header file.
This was used before __pkguniq to generate identifiers. Back then, I seemed
to think determinism was a problem and that randomness was desirable for
helping to ensure uniqueness between packages. That was a mistake; we
_want_ a deterministic system (which is far easier to debug and verify the
results of), we just want uniqueness.
DEV-14965
This modifies the XSLT-based compiler to generate ids that are expected to
be unique across packages. No such guarantee exists today; `generate-id()`
relies on the position of the node within a tree, which could easily be the
same across multiple compiler invocations for separate packages.
This situation seldom occurs, but has happened with increased frequency
lately in a system with >1000 packages. It is more likely to occur in
packages that are very similar to one-another or where the beginning of the
package is similar (such as packages used as configuration for taxes for
each individual state).
This derives a SHA-256 hash from the canonical package name (well, not
canonical acccording to TAMER, but close: without the leading slash),
truncating it to 32 bits. I used a birthday attack to estimate what the
size of this value ought to be: sqrt(2^32) = 65536, which is way more
packages than the poor XSLT-based compiler is going to handle.
If ever it needs to be increased due to conflicts, that is simple enough.
DEV-14965
This ensures that each metavariable defined within a template (a template
parameter) has, by the time that the template definition is ended, at least
one reference to each metavariable.
This has practical benefits---ensuring that you haven't forgotten to use a
metavariable; ensuring that you clean up when code is removed; and ensuring
that you didn't accidentally delete some reference that you didn't intend to
(at least in the case of _all_ references...)---but the rationale for it in
this commit is a bit different. More on that below.
This does introduce the negative effect of making it more difficult to
consume inputs without utilizing them, acting more like a relevant type
system (in terms of substructural type systems and with respect to
metavariables, at least). You can for now reference them in contexts that
would reasonably have no effect on the program or be optimized away, but in
the future it'd be nice to explicitly state "hey this isn't intended to be
used yet", especially if you're creating shells of templates, or trying to
maintain BC in a particular situation. But more on that in the future.
For now, the real reason for this change is because of how I intend for
template expansion to work: by walking the body. Rather than having to
check both the parameters of the template and then expand the body
separately, we can simply trust that each parameter is referenced. Then,
after rebinding metavariable references, we can perform the same check on
the expansion template to see if there were arguments provided that do not
correspond to parameters.
This also adds flexibility with parameters that are used
conditionally---we'll be able to have conditionally required parameters in
error reporting.
More information on this is coming, though; it'll be included in the docs of
the commit that introduces the changes.
DEV-13163
The number of files that had to be changed here emphasizes the need for a
better abstraction for indirection in the future. I just don't have the
time to do so yet.
This introduces the same type of indirection that was needed for abstract
bindings: we need to be able to distinguish between a metavariable
identifier's lexical value as a _string_ vs. a _reference_; the former is
how it was interpreted previously, which meant that interpolation would not
cause an edge to be created to the metavariable, which in turn means that
the metavariable was not referenced. This in turn blocked a change to
require that all metavariables be referenced.
I'm climbing back up my stash stack.
DEV-13163
This creates a new `AirDocAggregate` child parser and handled documentation
in a consistent way for all object types. The logic that was previously
object-specific now lives on the edge hooks that did not exist when doc
parsing was originally introduced.
This prepares for doc refs so that metavariables can be used for doc
generation (most notably for interpolated documentation strings in
templates).
This also introduces a new error type specifically for meta, giving guidance
to use `<text>`. I don't like that it builds in source language assumptions
in the help message provided, however I'd rather it be able to provide some
guidance to people, especially given that the XML source language is the
only one recognized by TAMER at the moment.
DEV-13163
Only internal modules ought to be able to mutate objects. This is important
now with the use of the `f::Map` trait, since traits don't allow for private
methods, which were previously the means of maintaining encapsulation.
DEV-13163
See included documentation for more information. This completes `MetaState`
inference, for now, until we are able to be notified when missing
identifiers acquire bindings.
DEV-13163
This moves us closer toward template expansion by allowing us to look at an
`Expr` and determine whether it requires any expansion at all. Naturally,
if something doesn't require expansion, we're able to duplicate it in the
tree without duplicating any data on the graph by simply adding a tree edge
to it. But, that's for a future commit; this just tracks the status.
Just as with some recent previous changes, we're not yet notifying `Expr`s
when an `Ident` is no longer `Missing`, and so order matters for now. This
will be rectified in the future.
DEV-13163
This prepares for the addition of more fields.
I don't want to sacrifice exhaustiveness checking, which is unfortunately
more verbose for structs when ignoring fields. This verbosity will be
adopted in certain situations, like shown in this patch, to ensure that
we're made aware of areas of the program that require our attention when
fields are added.
This also shows that I'll need a struct arm for `impl_mono_map!`.
DEV-13163
This removes the token of lookahead and just does what needs to be done in a
more clear manner. There is no room for interpretation in what this is
doing now, and `TplEnd` delegates to `close` just as this does.
DEV-13163
We are now able to have confidence that the graph is properly constructed
with or without a reference span depending on whether the edge is a tree or
cross edge. Consequently, the span is now a reliable indicator of whether
an edge is tree or cross in _all_ cases, not just in dynamic ones.
In fact, this did catch a couple cases where the spans were _not_ properly
applied.
This will in turn give me confidence moving forward with static analysis
based on the graph (and edge hooks).
I could go further than this by introducing a new span type in place of
`Option<Span>`, which would also allow me to get rid of having two methods
on `Asg`, but I want to move on for now; this can be cleaned up more later
on.
It's also worth noting that this explicit method-based distinction between
edge types also means that each caller will carefully consider how the
operation affects the graph. Previously, that consideration was framed very
differently: "do I need a contextual span or not?". That's not the right
question to ask.
DEV-13163
When this span was originally introduced for an edge, I wasn't sure whether
it may have use cases beyond just references. At this point, I'd rather be
explicit; it can evolve over time if need be.
There is nothing yet that prevents a span from being added to a tree edge,
though.
DEV-13163
This also hoists `?` out of the inner match arms to make control flow easier
to follow. I try to balance convenience with brutal explicitness, to make
control flow more obvious and make it easier to reason about what the system
is doing at every point in the program.
DEV-13163
This allows removing the `diagnostic_panic!` from the `Ident` edge handling
for `Tpl`.
I do need to create a generic sum type macro at some point, considering how
extensively these are used.
DEV-13163
Oh, identifiers. What a mess you make of the graph. As indirection tends
to do, I suppose.
This still needs some cleanup; I wanted to get something working first. In
particular:
- Common operations need refactoring;
- Having all this code in the edge definition makes for a mess;
- `Ident` references really ought not be returned by `Ident::definition`
since an identifier will never be a definition.
...I'm sure there's more.
DEV-13163
This renames the previous operation to `definition_narrow` and creates a new
`definition` that does not attempt to narrow. This is important for
distinguishing between a missing defintion and a definition of a given type,
and will be important now that we're beginning to reason about identifiers
that were resolved during parsing.
DEV-13163
I changed my mind from the previous commit---I do in fact want to inhibit
edge additions in some cases, to override what is added (e.g. to wrap a
template application in an Expr workspace).
The approach that I was trying to take was to override `from_oi` and return
it to the caller, but with all the dynamic edges, that ended up being a lot
more code than I feel was worth it; it was too complex. This is a much
simpler (and more flexible) approach---we simply carry out whatever graph
operations we want in `pre_add_edge` using the provided `asg` reference, and
then after we're done, simply omit `commit` on the original.
DEV-13163
This does two things:
1. Removes callback; it didn't add anything of practical value.
The operation will simply be performed as long as no error is provided
by the callee.
2. Consolodates three arguments into `ProposedRel`. This makes blocks in
`object_rel!` less verbose and boilerplate-y.
I'll probably implement `TplShape::Unknown` via the dynamic `Ident` `Tpl`
edge before continuing with any cleanup. This is getting pretty close to
reasonable for future implementations.
DEV-13163
Mike Gerwitz