This is the safe version of the existing intern_utf8_unchecked, and exists
as a performance optimization.
We're about to introduce a XIR reader, which is going to intern a _lot_ of
duplicate strings, since it will intern node and attribute names as
well. Given that, we do not want to spent a lot of time performing UTF-8
checks that have already been performed.
We know that, if an intern is in the pool, it's either already UTF-8 or that
check was bypassed when it was initially interned. Therefore, if we find an
existing symbol, that can be returned without having to perform any
check. Otherwise, we intern as we usually would after attempting to convert
the byte slice into a string.
This allows us to continue to have good performance for interning without
sacrificing safety for strings.
The intent of this is to demonstrate how significant of an impact checking
byte arrays for UTF-8 validity will have, since the existing tests do not
make that clear (a static string in Rust is always valid UTF-8).
These benchmarks show that the cost when re-interning an already existing
value is +50%.
This is important, because the new reader will be interning a _lot_ of
duplicate strings, whereas the existing reader operates on byte arrays
without interning unless necessary. And, when it does, it does so
unchecked. But we'd rather not do that, since we cannot guarantee that
those XML files are valid (and not modified in some way).
Upcoming commits will have what I think is a reasonable compromise to this,
based on the fact that we'll be encountering _many_ duplicate strings in
parsing XML files.
DEV-10920
This provides a child `raw` module that exposes a SymbolId representing the
inner value of each of the static newtypes. This is needed in situations
where the type must match and the type of the static symbol is not
important.
In particular, when comparing against runtime-allocated symbols in `match`
expressions.
It is also worth noting that this commit managed to hit a bug in Rustc that
was fixed on 10/1/2021. We use nightly, and it doesn't seem that this
occurred in stable, from bug reports.
- https://github.com/rust-lang/rust/issues/89393
- 5ab1245303
- Original issue: https://github.com/rust-lang/rust/issues/72476
The error was:
compiler/rustc_mir_build/src/thir/pattern/deconstruct_pat.rs:1191:22:
Unexpected type for `Single` constructor: <u32 as sym::symbol::SymbolIndexSize>::NonZero
thread 'rustc' panicked at 'Box<dyn Any>', compiler/rustc_errors/src/lib.rs:1146:9
This occurred because we were trying to use `SymbolId` as the type, which
uses a projected type as its inner value: `SymbolId<Ix: SymbolIndexSize>(Ix::NonZero)`.
This was not a problem with the static newtypes because their inner type was
simply `SymbolId<Ix>`, which is not projected.
This is one of the risks of using nightly.
But, the point is: if you receive this error, upgrade your toolchain.
Tbh, I was unaware that this was supported by tuple variants until reading
over the Rustc source code for something. (Which I had previously read, but
I must have missed it.)
This is more proper, in the sense that in a lot of cases we not only care
about how many values a tuple has, but if we explicitly match on them using
`_`, then any time we modify the number of values, it would _break_ any code
doing so. Using this method, we improve maintainability by not causing
breakages under those circumstances.
But, consequently, it's important that we use this only when we _really_
don't care and don't want to be notified by the compiler.
I did not use `..` as a prefix, even where supported, because the intent is
to append additional information to tuples. Consequently, I also used `..`
in places where no additional fields currently exist, since they may in the
future (e.g. introducing `Span` for `IdentObject`).
In particular, `name` needn't return an `Option`. `fragment` also returns a
copy, since it's just a `SymbolId`. (It really ought to be a newtype rather
than an alias, but we'll worry about that some other time.)
These changes allow us to remove some runtime panics.
DEV-10859
This moves the logic that sorts identifiers into sections into Sections
itself, and introduces XmleSections to allow for mocking for testing.
This then allows us to narrow the types significantly, eliminating some
runtime checks. The types can be narrowed further, but I'll be limiting the
work I'll be doing now; this'll be inevitably addressed as we use the ASG
for the compiler.
This also handles moving Sections tests, which was a TODO from the previous
commit.
DEV-10859
This is the appropriate place to be, now that we've begun narrowing the
types. We'll be able to do so further; this is just the first step.
This does not yet move the tests, but the code is still tested because it's
tightly coupled with `sort`. Those will move in the next commit(s).
DEV-10859
xmle sections will only ever contain an object of one type, so there is no
use in making this generic.
I think the original plan was to have this represent, generically, sections
of some object file (like ELF), but doing so would require a significant
redesign anyway, so it makes no sense. This is easier to reason about.
DEV-10859
This has always been a lowering operation, but it was not phrased in terms
of it, which made the process a bit more confusing to understand.
The implementation hasn't changed, but this is an incremental refactoring
and so exposes BaseAsg and its `graph` field temporarily.
DEV-10859
Sections, as written, are specific to xmle files.
I think the intent originally was to have this be more generic, but that
doesn't really make sense.
By explicitly coupling it with `xmle` files, that will allow us to turn this
into a proper lowering operation with its own validations that will allow
`xmle::xir` to do its job without having to validate anything itself.
This outputs enough information to be a little bit useful in the event of an
error. In the future, we'll want to provide a (likely non-Display)
implementation that provides line number and source file context with
the problem characters indicated, like Rust.
This is a significant departure from my original plans---this makes it
_easy_ to display symbol values, despite me not wanting that to occur unless
absolutely necessary.
The reality is, based on the design of the system, they will only occur in
these situations:
1. Writing to files;
2. Displaying errors;
3. Tests; or
4. People not following the design of the system.
The fourth one is the most risky as people begin to contribute in the
future, but the reality is that those can be fixed as they are encountered,
since if they're not showing up in a profiler, then they must not be causing
much of a problem.
This removes `SymbolStr` in favor of, simply, `&'static str`.
The abstraction provided no additional safety since the slice was trivially
extracted (and commonly, in practice), and was inconvenient to work with.
This is part of a process of relaxing lookups so that symbols can be
conveniently displayed in errors; rather than trying to prevent the
developer from doing something bad, we'll just rely on conventions, hope
that it doesn't happen, and if it does, address it either at that time or
when it shows up in the profiler.
The docs still need to be improved, but they can be touched as we go.
This concludes the initial development of XIR. That was much more involved
that I had originally intended, but the result is good.
DEV-10561
This generalizes it a bit and provides tests, which was always the intent;
the existing code was POC to determine if this could be done without
performance degradation (see that commit for more information).
The intent is to support the composition and decomposition of spans such
that (A, B) is as documented here. This only performs the trivial case for
the sake of providing a convenient API when the developer would otherwise
just type (S, S).
This is intended to represent the sections written to the final xmle file,
and there was unnecessary complexity in separating everything.
By reducing this IR further, we can begin to constrain its types to
eliminate some of the runtime panics and error checking we have/had in the
writer.
The new writer has reached parity of the old, with the exception of some
edge case explicit error handling that should never occur (which will be
added), and cleanup/docs.
Removing this flag now allows me to perform that cleanup without having to
worry about updating the now-old implementation.
I ran `tameld` with the new writer against our production system with
numerous programs and a significant number of test cases, and diff'd the old
and new xmle files, and everything looks good.
This is a significant milestone, in the sense that it is the culmination of
the past month or so of work to prove that an Iterator-based XIR will be
viable for the system.
This barely had any impact on the performance from the previous commit
reporting the profiling. This performs at least as well as the quick-xml
based writer. In isolated benchmarks, it performs better, but in the real
world, the linker spends most of its time reading xmlo files, and so minor
differences in writing do not have a significant overall impact.
With that said, a lot of cleanup and documentation is still needed. That is
the subject of the upcoming commits, before this writer can finalized.
The previous iterators had to be used in a certain order because they mixed
concerns, out of concern for performance. This attempts to chain even more
iterators to see how it may perform.
To be clear: this will be cleaned up. This was just an experiment.
Here were profiles on the average of 50 runs of linking our largest program:
Baseline, pre-XIR (with fragments removed from output) 0.8082
XIR writer, pre-ElemWrap, no #[inline] 0.7844s
XIR writer, ElemWrap, no #[inline] 0.7918s
XIR writer, ElemWrap, inlines in obj::xmle::xir 0.7892s
XIR writer, ElemWrap, inlines in obj::xmle::xir and ir::asg::section 0.7858s
XIR writer, ElemWrap, inline in only ir::asg::section 0.781s
Pre-ElemWrap, inlines in ir::asg::section 0.7772s
These profiles are difficult, because they hit the filesystem so much. I
write to /dev/null, but it reads 100s of xmlo files from disk.
It's clear that the impact is fairly modest and within a margin of error; as
such, I will continue down the path of writing code that's easier to grok
and maintain, since not doing so would be a micro-optimization relative to
the concerns of the rest of the system at this point.
But the purpose of all of this work was to determine whether an
iterator-based XIR would be viable. It seems to be competitive. I'll
finish up the writer reimplementation and move on.
Two reasons for this:
1. It's unnecessary, since it's the same ref, so long as we actually build
everything as part of the stage job; and
2. In our environment, the token used doesn't have access to pull from the
registry.
Fixing the latter item can be done at another time.
This contains some awkward coupling for opening and closing tags to reduce
the complexity of the `Iterator` types that must be manually
specified. That may be addressed shortly.
This was creating a heap-allocated `Vec` for each map symbol despite not
actually needing it. We do have multiple froms for return map values.
But by the time we may want this type of thing, we'll have a different IR
for it anyway.
See the docs for a much deeper discussion. In summary: traits do not
support static methods, and this is the workaround, which relies on unstable
nightly constant function features.
This implementation is tested using `qname_const!`, and will be utilized
with a new static type in a following commit.
This is to support two things:
1. Early switch to 2021 Edition, which is stable Oct 21; and
2. To make use of unstable const features.
The rationale is that switching to nightly does not really have any
significant downside for us, given that TAMER is used only by us and
the only risk is that unstable features may change a bit, which can be
mitigated with certain precautions.
The rationale for each unstable feature will be documented as they are used,
including documentation on what would be required to remove it and what
functionality would be lost / need to change in doing so.
This is far from fully documented; it's just a start. I'll document fully
once the implementation is done, to ensure I don't waste time documenting
things that may change.
Mike Gerwitz
Austin Schaffer